-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
Serde options refactor #506
Closed
Closed
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
0bc4564
rename Opt to ClientOpt
lovromazgon ad5657f
rename SerdeOpt to EncodingOpt, introduce different SerdeOpt
lovromazgon 7544ce4
Add EncodingOpt to Encode and related methods, introduce ID option
lovromazgon 1ade586
Add EncodingOpt to Decode and related methods
lovromazgon 6da9cf4
Merge remote-tracking branch 'upstream/master' into serde-options
lovromazgon 1bb340b
serde header option
lovromazgon 8aaf14f
introduce DynEncode and DynAppendEncode, simplify options
lovromazgon 8591c09
remove option ID
lovromazgon 18b30d9
rename append to encode
lovromazgon f0fd058
Merge branch 'master' into serde-options
lovromazgon 8038cd3
Merge remote-tracking branch 'upstream/master' into serde-options
lovromazgon 172db90
remove DynEncode method and replace it with a normal function
lovromazgon 2ffcf74
remove unused field in test cases
lovromazgon 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 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 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
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.
The last commit adds the ability to supply
EncodingOpt
toDecode
, simply to have parity withEncode
, but I'm not convinced if it makes sense. I included it to run it by you, but I can revert the change if needed.This is how it works. If you supply options
ID
andIndex
toDecode
it will findtserde
based solely on the supplied options, it will not decode the header. In fact, it will assume there is no header and that it's dealing only with the raw encoded bytes. What I don't like about this is thatSerde
can't encode bytes without a header unless you specify your ownSerdeHeader
implementation which skips prepending the header. It would allow you to track the ID and index separately from the encoded payload, although I'm not sure such a use case exists, so it might needlessly complicate the API.