-
Notifications
You must be signed in to change notification settings - Fork 37
Fix off-by-one bug in serde impl for Symbol types #91
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
Fix off-by-one bug in serde impl for Symbol types #91
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #91 +/- ##
==========================================
+ Coverage 60.60% 61.34% +0.74%
==========================================
Files 11 11
Lines 500 507 +7
==========================================
+ Hits 303 311 +8
+ Misses 197 196 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Robbepop
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.
Thanks for the intended fixes. I have reviewed the changes. Please fix the nits I have. Ideally we could even make the Symbol (de)serialization more generic by implementing (De)Serialize for all T: Symbol.
f4efa20 to
656e3d6
Compare
|
Feedback should be addressed now. Swapped to using We can't impl serde traits for all |
656e3d6 to
98b2451
Compare
|
Latest feedback applied! |
Robbepop
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.
@caspark Thanks a lot for this!
|
No worries, thanks for the quick reviews! |
The various Symbols' serde implementations had a bug that would cause their underlying index to increase by 1 each time they're serialized and deserialized, because the serialize impl didn't account for the fact that
Symbol::new()adds one to guarantee that the resulting index is non-zero.This fixes that and adds tests for it.
+ 1operation out ofSymbol::new()and into the deserialize impl for each Symbol, but I'm not sure if something else in this codebase relies onSymbol::new()adding 1.Fixes #88 .