-
Notifications
You must be signed in to change notification settings - Fork 6
Fix missing message id override in PPBC classes #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b088236
message_id should be available in all ppbc messages, split up type of…
lfse-slafleur 38ed38c
Write S2Message and S2MessageWithID as type aliases.
lfse-slafleur 210dd8e
Ensure that setuptools-scm and typing-extensions are available.
lfse-slafleur 4611cc3
to_dict should take a mode and add some unit tests.
lfse-slafleur 4cb9416
json.dumps should work for to_dict if mode is set to json
lfse-slafleur f7c8745
Split to_dict with json and python modes into separate functions.
lfse-slafleur 99c4ba0
Fix typo.
lfse-slafleur File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import json | ||
| import unittest | ||
| import datetime | ||
| import uuid | ||
|
|
||
| from s2python.validate_values_mixin import S2MessageComponent | ||
|
|
||
|
|
||
| class MockS2Message(S2MessageComponent): | ||
| some_uuid: uuid.UUID | ||
| some_str: str | ||
| some_float: float | ||
| some_datetime: datetime.datetime | ||
| some_timedelta: datetime.timedelta | ||
|
|
||
|
|
||
| def example_message() -> MockS2Message: | ||
| return MockS2Message(some_uuid=uuid.uuid4(), | ||
| some_str='asdaa', | ||
| some_float=3.14, | ||
| some_datetime=datetime.datetime.now(), | ||
| some_timedelta=datetime.timedelta(hours=1, minutes=1)) | ||
|
|
||
|
|
||
| class TestS2MessageComponent(unittest.TestCase): | ||
| def test__to_json__okay(self): | ||
| # Arrange | ||
| message = example_message() | ||
|
|
||
| # Act | ||
| json_str = message.to_json() | ||
|
|
||
| # Assert | ||
| message_json = json.loads(json_str) | ||
| self.assertEqual(message_json['some_uuid'], str(message.some_uuid)) | ||
| self.assertEqual(message_json['some_str'], message.some_str) | ||
| self.assertEqual(message_json['some_float'], message.some_float) | ||
| self.assertEqual(message_json['some_datetime'], message.some_datetime.isoformat()) | ||
| self.assertEqual(message_json['some_timedelta'], 'PT1H1M') | ||
|
|
||
| def test__to_dict__okay(self): | ||
| # Arrange | ||
| message = example_message() | ||
|
|
||
| # Act | ||
| message_dict = message.to_dict() | ||
|
|
||
| # Assert | ||
| self.assertEqual(message_dict['some_uuid'], message.some_uuid) | ||
| self.assertEqual(message_dict['some_str'], message.some_str) | ||
| self.assertEqual(message_dict['some_float'], message.some_float) | ||
| self.assertEqual(message_dict['some_datetime'], message.some_datetime) | ||
| self.assertEqual(message_dict['some_timedelta'], message.some_timedelta) | ||
|
|
||
| def test__to_json_dict__okay(self): | ||
| # Arrange | ||
| message = example_message() | ||
|
|
||
| # Act | ||
| message_dict = message.to_json_dict() | ||
|
|
||
| # Assert | ||
| json.dumps(message_dict) | ||
| self.assertEqual(message_dict['some_uuid'], str(message.some_uuid)) | ||
| self.assertEqual(message_dict['some_str'], message.some_str) | ||
| self.assertEqual(message_dict['some_float'], message.some_float) | ||
| self.assertEqual(message_dict['some_datetime'], message.some_datetime.isoformat()) | ||
| self.assertEqual(message_dict['some_timedelta'], 'PT1H1M') |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MauriceHendrix What is the issue you have with subclassing? This allows us to refer to S2 messages which have a message id with
S2MessageWithIDand all S2 messages (even those without a message id) withS2Message. However,S2MessageWithIDis 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-2149287ebe9503504f407458b92eea4f7e4bb7cffa11e936cab5730f40bf9a13R72There 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.
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)