-
Notifications
You must be signed in to change notification settings - Fork 2
Type widening for accepted records #14
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
base: main
Are you sure you want to change the base?
Conversation
elenagaljak-db
left a comment
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.
Lgtm, although I am starting to second guess the decision of sending JSON records as dictionaries
Signed-off-by: teodordelibasic-db <[email protected]>
23e7c2e to
b457476
Compare
| offset1 = await ack1 | ||
| self.assertEqual(offset1, 0) | ||
|
|
||
| ack2 = await stream.ingest_record(json.dumps({"device_name": "device2", "temp": 22, "humidity": 55})) |
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 would be good to add tests for every value type we support
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, thanks.
zerobus/sdk/aio/zerobus_sdk.py
Outdated
| elif self._options.record_type == RecordType.JSON: | ||
| if not isinstance(record, dict): | ||
| if isinstance(record, dict): | ||
| serialized_record = json.dumps(record).encode("utf-8") |
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.
Should we try catch json dumps? Example if user provides nested custom classes etc, this will raise an exception. Maybe we want to wrap it into our error handling.
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, thanks.
Signed-off-by: teodordelibasic-db <[email protected]>
Signed-off-by: teodordelibasic-db <[email protected]>
What changes are proposed in this pull request?
Introduce type widening for the type of records the
ingest_recordmethod accepts to dict/string for JSON and Message/bytes for Protobuf.How is this tested?
New tests, manual and examples.