Repository navigation
Improve externally facing class names - #166
jacob-williamson wants to merge 13 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
==========================================
- Coverage 98.67% 98.66% -0.02%
==========================================
Files 23 23
Lines 1287 1271 -16
==========================================
- Hits 1270 1254 -16
Misses 17 17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, I think the change is probably a bit clearer but more docstrings would make it even clearer still.
|
|
||
| def to_response(self) -> "BlueapiCallResponse": | ||
| return BlueapiCallResponse.from_blueapi_call(self) | ||
| def to_response(self) -> "QueueBlueapiCall": |
There was a problem hiding this comment.
Should: This method name now doesn't make much sense
|
|
||
|
|
||
| class BlueapiCallResponse(BaseModel): | ||
| class QueueBlueapiCall(BaseModel): |
There was a problem hiding this comment.
Should: Can we have a docstring here explaining how this differs from BlueapiCall? It also feels weird that we have two classes with so much repeated info, will #125 tidy this up or maybe we need some other inheritance to tidy it?
There was a problem hiding this comment.
The reason I didn't use inheritance is because these externally facing classes are intended as snapshot copies of the internally used classes. So none of the fields have defaults for example, as it is expected they are all provided by an instance of the internal class. And properties on the internal class are regular fields on the external one as the internal class calculates them and populates. Maybe a load of over complication for not much benefit, will see if I can refactor
There was a problem hiding this comment.
Looking into it a bit more and remembering things, another reason I didn't use inheritance was because the internal classes have some methods on it which are meaningless for instances passed to plugins, when they are only intended in that case as a blob of information (like BlueapiCall.put_in_progress, which does nothing if called in a plugin but may be confusing to have)
There was a problem hiding this comment.
Have refactored a bit, could also roll #125 into the same PR?
|
|
||
|
|
||
| class TaskWithPosition(BaseModel): | ||
| class QueueTask(BaseModel): |
There was a problem hiding this comment.
Should: Similar to https://github.com/DiamondLightSource/daq-queuing-service/pull/166/changes#r4455793608, a docstring would be good, and we should think about if there's some class hierarchy here
| """ | ||
| return True | ||
|
|
||
| def process_running_task( |
There was a problem hiding this comment.
Must: This is a change from https://github.com/DiamondLightSource/daq-queuing-service/pull/152/changes, right? Can we make sure we have this PR as just the one thing?
There was a problem hiding this comment.
Yeah must have had an dirty base branch, will fix
| tiled_ids, scan_ids = get_tiled_and_scan_ids(self._tiled_client, call) | ||
| should_pause = self._converter.pause_on_complete_blueapi_call( | ||
| call.model_copy(), result | ||
| call.to_response(), result |
There was a problem hiding this comment.
Should: Is this also a change from https://github.com/DiamondLightSource/daq-queuing-service/pull/152/changes? It's not clear how this is part of changing the name.
There was a problem hiding this comment.
Some of these have changed due to typing changes on the plugin, I've made the plugin always receive QueueBlueapiCall instead of BlueapiCall as they never need any of the state change methods on BlueapiCall
1e67f33 to
51e827f
Compare
Changes
TaskWithPositiontoQueueTaskandBlueapiCallResponsetoQueueBlueapiCall, to improve clarity when using these types in other repos.