-
Notifications
You must be signed in to change notification settings - Fork 7
Serialize thread command #51
Serialize thread command #51
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 1.0.x_dev #51 +/- ##
=============================================
+ Coverage 73.43% 74.06% +0.63%
=============================================
Files 18 18
Lines 1611 1635 +24
=============================================
+ Hits 1183 1211 +28
+ Misses 428 424 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9711553 to
31f8003
Compare
|
|
||
| # List of all objects serializable via the serializer | ||
| SERIALIZABLE = Union[bytes, str, int, float, complex, list, NDArray, SerializableBase] | ||
| SERIALIZABLE = Union[None, bytes, str, int, float, complex, list, NDArray, SerializableBase] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As None as been defined as a class inheriting from SerializerBase, I'm not sure it's necessary to add it here. Indeed for ThreadCommand or other class elsewhere, thay are not added in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, None is still the built-in and not a subclass of SerializableBase.
There is a class (NoneSerializaDeserialize), which is based on that other class, but is just a container for the serialization.
Similar to the other built-in types bytes, str, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well yes but all these builtins are also encapsulated so is it necessary to add them namely in the SERIALIZABLE variable? You introduced this for pylecco I think, could you recall me what is it for exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SERIALIZABLE is there as a type to use for type hints:
def whatever(obj: SERIALIZABLE) -> bytes:
...here, a type checker (like mypy, pylance...) will complain if obj is not any of the classes in SERIALIZABLE. The builtins are not a subclass of SerializableBase. Therefore, they must be explicitly included in SERIALIZABLE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is mainly used in this pymodaq_utils repo for typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this typing to two more methods (feel free to revert):
Now get_apply_serializer(not_serializable_object) will complain in a type checker, that not_serializable_object is neither one of the builtins nor a subclass of the SerializableBase (if it is so).
The previous Any won't complain at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
|
||
| class SerializableTypes(Enum): | ||
| """Type names of serializable types""" | ||
| NONE = "NoneType" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember either how this is needed... but from what I see other types defined elsewhere as dwa, dte or parameterWithPath are defined in here. But then when commenting this object: al tests pass... I'm not sure what we should do about it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should comment it in here, check the director/actor communication is still fine...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it was used earlier, before the type() value was used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think so too. So comment it lefting a comment saying this just in case...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| # List of all objects serializable via the serializer | ||
| SERIALIZABLE = Union[bytes, str, int, float, complex, list, NDArray, SerializableBase] | ||
| SERIALIZABLE = Union[None, bytes, str, int, float, complex, list, NDArray, SerializableBase] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
proper implementation of ThreadCommand serialization instead of the self written part in PyMoDAQ/PyMoDAQ#405
This allows sending ThreadCommands via LECO, so should make status changes easier to transmit.
I'm not sure, whether I adjusted the
SerializableTypescorrectly.