Hygiene fixes for ser / deser macros #1176
Open
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
github-actions
bot
added
the
semver-checks-breaking
|
See the following report for details: cargo semver-checks output |
|
What is this failure in min_rust, why would Rust version have any impact here ?!! |
|
All of How could this possibly fail and why does it mention "tracing"???? |
The older version of tracing does not work without implicit prelude, which breaks the hygiene tests after changes.
It contains only 2 submodules: serialize and deserialize. In order to synchronize the paths of those modules with the scylla crate, the types module is made private, and its submodules are re-exported. fixup
This is required in order to get rid of types module.
Those are virtually top-level modules after previous commits, because they are re-exported that way in lib.rs. The only thing needed is to physically move the files.
It is now empty, so can be safely removed.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Macros need to use fully qualified paths in order to maintain hygiene.
Those constructors are needed for users that want to utilize our type ser / deser functionality more directly.
There is no need to create whole PreparedMetadata in order to create RowSerializationContext.
New version of the test verifies ser / deser identity for both row and value macros.
This test was checking only the most basic scenario: a structure that is serialized / deserialized by name matching, and has no field attributes. Adding more advanced scenarios uncovered many hygiene issues in the macros - those issues were fixed in previous commits.
Lorak-mmk
force-pushed
the
migrate-higiene-test
branch
from
e87f598 to
4c13236
Compare
|
Managed to fix the problem. Turns out that older version of tracing, which we had in Cargo.lock.msrv, doesn't work without implicit prelude. |
wprzytula
reviewed
Jan 23, 2025
| @@ -6,7 +6,7 @@ use scylla_cql::frame::request::query::PagingState; | |||
| use scylla_cql::frame::request::SerializableRequest; | |||
| use scylla_cql::frame::response::result::ColumnType; | |||
| use scylla_cql::frame::{request::query, Compression, SerializedRequest}; | |||
| use scylla_cql::types::serialize::row::SerializedValues; | |||
| use scylla_cql::serialize::row::SerializedValues; | |||
There was a problem hiding this comment.
commit scylla_cql: Inline types module: commit message contains stray fixup word.
scylla/tests/integration/hygiene.rs
Outdated
Comment on lines
100
to
118
| #[test] | ||
| fn test_value_ser_deser() { | ||
| use crate::utils::setup_tracing; | ||
| use ::bytes::Bytes; | ||
| use ::core::primitive::u8; | ||
| use ::std::assert_eq; | ||
| use ::std::convert::Into; | ||
| use ::std::option::Option::Some; | ||
| use ::std::string::ToString; | ||
| use ::std::vec; | ||
| use ::std::vec::Vec; | ||
| use ::tracing::info; | ||
| use _scylla::deserialize::DeserializeValue; | ||
| use _scylla::deserialize::FrameSlice; | ||
| use _scylla::frame::response::result::{ColumnType, CqlValue}; | ||
| use _scylla::serialize::value::SerializeValue; | ||
| use _scylla::serialize::writers::CellWriter; | ||
|
|
||
| setup_tracing(); |
There was a problem hiding this comment.
❓ 🔧 You set up tracing in this test but not in the other.
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.
Removing old serialization / deserialization APIs requires migrating the hygiene integration test - this is one of very few places that still uses old APIs.
This makes such migration a prerequisite for #1167
While migrating the test I found that our new macros actually have many problems with hygiene. Who could expect that if we don't test something then it won't work...
Apart from hygiene fixes in macros one more change was required: moving
types/serializeandtypes/deserializemodules inscylla_cqlto, respectively,serializeanddeserialize.In other words, inlining
typesmodule. Why is it required? The tests assume that required traits are exposed under the same paths inscyllaandscylla_cql, and the ability to make suchassumption is present in the documentation of the macros.
This PR fixes all the hygiene problems that I found, inlines
typesmodule, and migrates the hygiene test to new APIs.Apart from that I significantly extended the test. Before it tested only most basic scenario, but our new macros have many variants and possible field attributes.
Doing this proved to be good idea, because I found and fixed even more issues in macros.
Ref: #1167
Pre-review checklist
I have provided docstrings for the public items that I want to introduce.I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.