serialize: add more tests and docstrings #878
Merged
Conversation
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
piodul
force-pushed
the
docstrings-and-tests
branch
from
95acafa to
5511cea
Compare
Lorak-mmk
requested changes
Dec 13, 2023
There was a problem hiding this comment.
Fix the situation in this commit by
enforcing a consistent naming scheme:
- If something from missing in Rust data but required by the CQL type,
use<RustStuff>MissingFor<CqlStuff>.- If something is present in Rust data but missing from the CQL type,
useNoSuch<CqlStuff>orNo<CqlStuff>WithName.
Maybe this scheme should be documented in code so that it is enforced if any new errors / variants are added in the future?
There was a problem hiding this comment.
I'm not sure where would be the best way to put it and how to formulate it... Let's leave it for a follow up for now.
When serializing a Rust struct / CqlValue::UserDefinedType to a CQL UDT, or a Rust struct / BTreeMap / HashMap as a list of named values, there can be a fields/columns mismatch between the Rust and CQL definition and a field/column can be present in one or missing in another. Current error kind variants have very confusing or plainly wrong names/descriptions/messages. Fix the situation in this commit by enforcing a consistent naming scheme: - If something from missing in Rust data but required by the CQL type, use `<RustStuff>MissingFor<CqlStuff>`. - If something is present in Rust data but missing from the CQL type, use `NoSuch<CqlStuff>` or `No<CqlStuff>WithName`. Apart from that, fix the docstrings and the error messages.
The code that adapts `ValueList` to `SerializeRow` and rewrites named values to regular values based on the context didn't use to check whether there are some superfluous values that do not match to any of the bind markers. Fix this.
Add a dedicated error variant to ValueToSerializeCqlAdapterError, returned in case when the legacy implementation tried to serialize but overflowed the maximum allowed size by the protocol. Previously, the `ValueTooBig` error defined in frame::value would be returned, which was inconsistent with other error cases for the `Value` -> `SerializeCql` translation code.
In case when serialization of one of the fields fails, the tuple should return an error indicating that serialization of the tuple failed, and nest the error returned by field serialization inside it. The error used to have the wrong ColumnType put into it - a loop variable containing the ColumnType of the field shadowed the function argument containing the ColumnType of the whole tuple. Fix the issue by renaming the loop variable and un-shadowing the function argument as a result.
It's a good practice to implement common traits for public types, and some of them will be used in the tests introduced in the next commits.
The PR that introduced impls of the SerializeRow/SerializeCql for the types that used to implement the old traits relied on the tests in `value_tests.rs` - they were extended with more test cases and modified to run on both the old and the new traits. However, this was only done for the tests that serialize stuff without causing errors. The new impls return errors that are much more detailed than what the old ones used to return, and - as evidenced by the bugs fixed in previous commits - badly needed their own set of tests. Add such a set. All different kinds of built-in errors should be covered, with the exception of size overflow errors which would require allocating huge amounts of memory to trigger them.
Put the `#![warn(missing_docs)]` attribute at the top of the `types::serialize` module to trigger warnings for undocumented public items, and then add docstrings to the items reported by the warnings.
piodul
force-pushed
the
docstrings-and-tests
branch
from
5511cea to
7cf355b
Compare
Lorak-mmk
approved these changes
Dec 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR aims to improve the quality of the serialize module:
types/serializemodule and adds a compile-time warning about missing public items in order to prevent regressions.Pre-review checklist
I have adjusted the documentation in./docs/source/.I added appropriateFixes:annotations to PR description.