-
Notifications
You must be signed in to change notification settings - Fork 138
Added serde Derive Traits to Scylla Value Types. #1413
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
|
See the following report for details: cargo semver-checks output |
|
Please follow the procedure described in |
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.
Sorry for the delay! Generally looks good. There are some changes needed.
- Please rebase on main. We no longer have Cargo.lock.msrv thanks to Rust 2024, so your last commit becomes redundant.
- Every commit should be an atomic change that passes checks and tests, and has proper description. Your third and fourth commit are fixing issues introduced by eralier commits. Instead of having new commits for that, the earlier commits should be edited to not introduce those issues in the first place. This approach also makes the review easier - we can review each commit separately, and not see issues that are fixed in some later commit.
- I'm not familiar with serde, so it is not obvious to me if the functionality you introduced works properly. Please add unit tests that demonstrate what this PR allows the user to do.
Changes
Added serde's Serialize and Deserialize derive to some scylla value types.
Fixes: #1331
[ ] I added relevant tests for new features and bug fixes.[ ] I have provided docstrings for the public items that I want to introduce../docs/source/.[ ] I added appropriateFixes:annotations to PR description.