Fix fj serialization #682
Fix fj serialization #682
Conversation
This required 3 changes:
1. Import serde::{Serialize, Deserialize}.
2. Implement {Serialize, Deserialize} for Sketch.
3. Enable the "derive" feature for serde.
kamirr
force-pushed
the
fix_fj_serialization
branch
from
21f87a1 to
50238b3
Compare
There was a problem hiding this comment.
Thank you for this pull request, @kamirr!
I totally missed that the feature wasn't enabled in the CI build when merging this. This shouldn't have slipped through.
just builddoesn't run the tests I've added tofj, despite them running fine viacargo test. Am I missing something?
It looks like the fj crate is missing from default-members in the top-level Cargo.lock file. I'd say that is the reason.
Should I commit changes to
Cargo.lock?
Yes, definitely! I've recently added a section on this to CONTRIBUTING.md.
I've left one comment, but that's just a minor nitpick. Feel free to address that in a follow-up PR, if you're up for it, or don't, if you're not 😄
I've opened the following issues in response to the shortcomings that your work here uncovered:
| [dev-dependencies] | ||
| serde_json = "1.0.81" |
There was a problem hiding this comment.
Minor nitpick: I think it would be a bit more clear to move this to the end of the file. Otherwise we have dependencies inter-mixed with dev-dependencies.
Followup on PR #682 (fj serialization)
Fixes #681.
Two things I'm unsure about:
just builddoesn't run the tests I've added tofj, despite them running fine viacargo test. Am I missing something?Cargo.lock?