#556 Add value_serializer parameter to table and collection definit… #659
Conversation
…ection definitions
faust/tables/base.py
Outdated
| # Fallback to json | ||
| self.key_serializer = self._serializer_from_type(self.key_type) | ||
| self.value_serializer = self._serializer_from_type(self.value_type) | ||
| self.value_serializer = value_serializer # Add this line |
There was a problem hiding this comment.
why do you comment add this line everywhere? seems like a rather invaluable comment
There was a problem hiding this comment.
That comment made me remember what I should do. Can I remove these comments if that works for you?
There was a problem hiding this comment.
I'd recommend it as this is not useful anymore IMO
| window: Optional[WindowT] = None, | ||
| partitions: Optional[int] = None, | ||
| help: Optional[str] = None, | ||
| value_serializer: str = None, |
There was a problem hiding this comment.
Sorry I did only look into this quickly before but the typing is off here.
Why isn't it Optional[CodecArg] like in the Collections base class?
| # Fallback to json | ||
| self.key_serializer = self._serializer_from_type(self.key_type) | ||
| self.value_serializer = self._serializer_from_type(self.value_type) | ||
| self.value_serializer = value_serializer |
There was a problem hiding this comment.
I am not deep in to Tables yet as I have not used them extensively, but it looks like we do lose convenience behaviour here, as the _serializer_from_type functions translates us bytes to raw and sets the default json.
This seems to be a breaking change. can you elaborate the behaviour?
There was a problem hiding this comment.
Thank you for your review. I will make changes
2 participants
Issue #556
This pull request includes several changes to the
faustlibrary, specifically in thefaust/app/base.pyandfaust/tables/base.pyfiles. The most important changes involve the addition of avalue_serializerparameter to various methods and the correction of a minor typo.Additions of
value_serializerparameter:faust/app/base.py: Addedvalue_serializerparameter to theTablemethod and included it in the method's body. [1] [2]faust/tables/base.py: Addedvalue_serializerparameter to the__init__method of theTableclass and used it in place of the previous value serializer assignment. [1] [2]faust/tables/base.py: Includedvalue_serializerin the_new_store_by_urlmethod.Minor typo correction:
faust/app/base.py: Corrected a typo in the comment for_app_tasksinitialization.EDIT
Title
Fix
value_serializerIssue insend_soonMethod and Add Error Handling forNoneEvent in_send_changelogDescription
This PR resolves an issue where the
value_serializerwas not properly set in thesend_soonmethod, leading to anAssertionErrorduring testing. Additionally, it enhances error handling in the_send_changelogmethod by explicitly raising aRuntimeErrorif theeventparameter isNone.These changes ensure more robust behavior and prevent silent failures when processing changelog events.
Changes Made
File:
faust/tables/base.pyvalue_serializeris correctly set in thesend_soonmethod._send_changelogto check forNoneevents and raise aRuntimeErrorif encountered.key_serializerandvalue_serializerto'json'in_send_changelogto prevent serialization issues.File:
tests/unit/tables/test_base.pytest_send_changelogmethod now verifies thatvalue_serializeris correctly set.RuntimeErroris raised wheneventisNonein_send_changelog.Testing
tests/unit/tables/test_base.pyto confirm no regressions.value_serializerdefaults to'json'and is correctly assigned insend_soon._send_changelograises aRuntimeErrorwheneventisNone.Related Issues
Fixes #556
Checklist
✅ Code changes are complete.
✅ Unit tests have been updated to cover the changes.
✅ All tests pass locally.
✅ Documentation has been updated (if applicable).