Fix missing message id override in PPBC classes #134
Conversation
… messages with a message id vs without and ensure that to_dict is json serializable.
MauriceHendrix
left a comment
There was a problem hiding this comment.
looks plausable and if i understand it correctly the trick with message-id is noce, but I can't really oversee the consequences of the to_dict change
| PowerMeasurement, | ||
| ] | ||
|
|
||
| S2Message: TypeAlias = Union[ |
There was a problem hiding this comment.
Does this fix the issues we're having with subclassing and the message_id?
Do we have a testcase for it?
There was a problem hiding this comment.
@MauriceHendrix What is the issue you have with subclassing? This allows us to refer to S2 messages which have a message id with S2MessageWithID and all S2 messages (even those without a message id) with S2Message. However, S2MessageWithID is currently not a type that is a result from any function in this library so it is up to the user to actually differentiate somewhere in their code if the message they are currently refering to is one with a message id or if it is unknown. It is a bit hard to test given it is a typing alias, but I provided some usage here: https://github.com/flexiblepower/s2-python/pull/134/files#diff-2149287ebe9503504f407458b92eea4f7e4bb7cffa11e936cab5730f40bf9a13R72
There was a problem hiding this comment.
Now that I think about it I think this was more of a problem in our Java code (which we did solve a while ago)
|
@MauriceHendrix In response to your latter comment: Yeah you are right. I thought we could get away with it as I was assuming |
VladIftime
left a comment
There was a problem hiding this comment.
Looks good! Thanks for catching the issue
|
Other than the comment about the to_dict, looks perfectly fine. |
No description provided.