Updated to cbor2 5.5.0, and addressed review comments #107
Conversation
Only patch versions will be automatically updated
There was a problem hiding this comment.
Looks generally good.
However, you missed a few calls to send_client_done() in the tests (are there no calls anywhere else that needed changing??), and the annotations and initialization of StepType don't seem quite right, but neither of those turn out to be catastrophic, thanks to the magic that is Python. 😁
And, as is typical, I have other suggestions which you can take for what they are worth.
| @@ -143,40 +144,43 @@ def run_server_read_loop(self) -> None: | |||
| while True: | |||
| # Decode the message | |||
| runtime_msg = self.decoder.decode() | |||
| msg_id = runtime_msg["id"] | |||
| run_id = runtime_msg["run_id"] | |||
| msg_id = runtime_msg.get("id", None) | |||
There was a problem hiding this comment.
It's not a problem, but None is the default value for the second argument to get(), so you don't really need to specify it explicitly.
| @@ -265,7 +266,7 @@ def test_broken_step(self): | |||
| client.send_client_done() | |||
There was a problem hiding this comment.
The call to client.read_single_result() at line 265 (which I cannot cite here, because you didn't change it...) is mis-coded; fortunately, the call raises an exception before it returns, otherwise you would get a TypeError during the assignment.
Ditto lines 285 and 305.
| @@ -265,7 +266,7 @@ def test_broken_step(self): | |||
| client.send_client_done() | |||
| self.assertIn("abcde", str(context.exception)) | |||
| finally: | |||
| self._cleanup(pid, stdin_writer, stdout_reader) | |||
| self._cleanup(pid, stdin_writer, stdout_reader, True) | |||
There was a problem hiding this comment.
I would have used the argument keyword for the last parameter: can_fail=True.
| self.send_error_message(run_id, step_fatal=True, server_fatal=False, | ||
| error_msg=f"Error while calling step {run_id}/{step_id}:" |
There was a problem hiding this comment.
If you had it all to do over, I would suggest putting the fixed/required parameters first (i.e., run_id and error_msg) and passing the "option" (boolean) parameters by keyword. Positioning the keyword arguments at the end of the list allows you to omit the keyword from the error_msg parameter (where it doesn't provide enough positive value to make up for the space it takes), and, if you define suitable default values for the booleans, you can omit them from at least half the calls.
| outputs: Dict[ID_TYPE, StepOutputType] | ||
| signal_handler_method_names: List[str] | ||
| signal_handlers: Dict[ID_TYPE, SignalHandlerType] | ||
| signal_emitters: Dict[ID_TYPE, SignalSchema] | ||
| initialized_object_data: Dict[str, _StepLocalData] = {} # Maps run_id to data | ||
| initialization_lock: threading.Lock = threading.Lock() | ||
| signal_emitters: Optional[Dict[ID_TYPE, SignalSchema]] | ||
| initialized_object_data: Dict[str, _StepLocalData] # Maps run_id to data | ||
| initialization_lock: threading.Lock |
There was a problem hiding this comment.
input, outputs, signal_handlers, and signal_emitters are attributes of the parent class...you probably shouldn't be including annotations for them here (they are already annotated in the parent class definition).
| super().__init__(id, input, outputs, signal_handlers=None, signal_emitters=signal_emitters, display=display) | ||
| self._handler = handler | ||
| self._step_object_constructor = step_object_constructor | ||
| self.signal_handler_method_names = signal_handler_method_names | ||
| self.initialized_object_data = {} | ||
| self.signal_emitters = signal_emitters | ||
| self.display = display | ||
| self.initialization_lock = threading.Lock() |
There was a problem hiding this comment.
Since signal_emitters and display are attributes of the parent class, you probably shouldn't be initializing them here, instead the super().__init__() call should handle that.
Also, is signal_handler_method_names related to signal_handlers in some way?...in particular, should they both be attributes of the same class?
Changes introduced with this PR
This PR updates to cbor2, which is annoyingly a breaking change for a minor version.
It also addresses the comments that Webb left on my prior PR.
I have not tested this with plugins, but the tests pass, and theoretically I fixed things and didn't break anything. 😄
By contributing to this repository, I agree to the contribution guidelines.