Skip to content
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

scylla/lib.rs: Remove stars in serialization frameworks imports #1090

Merged
merged 3 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions scylla-cql/src/types/serialize/batch.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
//! Contains the [`BatchValues`] and [`BatchValuesIterator`] trait and their
//! implementations.

// Note: When editing above doc-comment edit the corresponding comment on
// re-export module in scylla crate too.

use crate::frame::value::{LegacyBatchValues, LegacyBatchValuesIterator};

use super::row::{RowSerializationContext, SerializeRow};
Expand Down
3 changes: 3 additions & 0 deletions scylla-cql/src/types/serialize/row.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Contains the [`SerializeRow`] trait and its implementations.

// Note: When editing above doc-comment edit the corresponding comment on
// re-export module in scylla crate too.

use std::borrow::Cow;
use std::collections::{BTreeMap, HashSet};
use std::fmt::Display;
Expand Down
3 changes: 3 additions & 0 deletions scylla-cql/src/types/serialize/value.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Contains the [`SerializeValue`] trait and its implementations.

// Note: When editing above doc-comment edit the corresponding comment on
// re-export module in scylla crate too.

use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use std::fmt::Display;
use std::hash::BuildHasher;
Expand Down
3 changes: 3 additions & 0 deletions scylla-cql/src/types/serialize/writers.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Contains types and traits used for safe serialization of values for a CQL statement.

// Note: When editing above doc-comment edit the corresponding comment on
// re-export module in scylla crate too.

use thiserror::Error;

use super::row::SerializedValues;
Expand Down
72 changes: 69 additions & 3 deletions scylla/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@
//! # Ok(())
//! # }
//! ```
//! But the driver will accept anything implementing the trait [SerializeRow]
//! (crate::serialize::row::SerializeRow)
//! But the driver will accept anything implementing the trait [SerializeRow].
//!
//! ### Receiving results
//! The easiest way to read rows returned by a query is to cast each row to a tuple of values:
Expand Down Expand Up @@ -131,8 +130,75 @@ pub mod frame {
}

/// Serializing bound values of a query to be sent to the DB.
Lorak-mmk marked this conversation as resolved.
Show resolved Hide resolved
// Note: When editing comment on submodules here edit corresponding comments
// on scylla-cql modules too.
pub mod serialize {
pub use scylla_cql::types::serialize::*;
pub use scylla_cql::types::serialize::SerializationError;
/// Contains the [BatchValues][batch::BatchValues] and [BatchValuesIterator][batch::BatchValuesIterator] trait and their
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I pointed out in other comment, [BatchValues][batch::BatchValues] does not render correctly for me:
image

/// implementations.
Comment on lines +137 to +138
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so [][] works as well? I only knew about []().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you will notice that my third commit in this PR removes an occurence of []() because it was rendered incorrectly.

I honestly have no idea how it works. This is the only syntax that I found that seems to do what it should. Why? I have no idea. I read the rustdoc chapter about it: https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html
but it seems incorrect. For example, I would expect those variants to work and link and render correctly:

[batch::BatchValues]
[`batch::BatchValues`]

but they don't.

Copy link
Contributor

@muzarski muzarski Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you will notice that my third commit in this PR removes an occurence of []() because it was rendered incorrectly.

It was not working because [] and () were on separate lines (separated by //!):

//! But the driver will accept anything implementing the trait [SerializeRow]
//! (crate::serialize::row::SerializeRow)

I even checked out locally to see, because at first glance, this looked correct to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. The version after my fix works too, right? At least it did when I checked locally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for me it renders correctly as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like things that are highlighted in vscode, do not render correctly for cargo doc and vice-versa...
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@muzarski do you know why

[batch::BatchValues]
[`batch::BatchValues`]

don't work? This of course applies to all the other submodules in this place.

So for me, these do not work in vscode, but render correctly in cargo docs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[]() should work everywhere, so let's just stick to it. It's the ordinary Markdown format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[]() should work everywhere, so let's just stick to it. It's the ordinary Markdown format.

I'll do it. Still it's sad that this is so difficult to get right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I can change it but both [BatchValues](batch::BatchValues) and [BatchValues][batch::BatchValues] are not highlighted in VSCode (and both render corrrectly), so there is no difference.

pub mod batch {
// Main types
pub use scylla_cql::types::serialize::batch::{
BatchValues, BatchValuesFromIterator, BatchValuesIterator,
BatchValuesIteratorFromIterator, TupleValuesIter,
};

// Legacy migration types - to be removed when removing legacy framework
pub use scylla_cql::types::serialize::batch::{
LegacyBatchValuesAdapter, LegacyBatchValuesIteratorAdapter,
};
}

/// Contains the [SerializeRow][row::SerializeRow] trait and its implementations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

pub mod row {
// Main types
pub use scylla_cql::types::serialize::row::{RowSerializationContext, SerializeRow};

// Errors
pub use scylla_cql::types::serialize::row::{
BuiltinSerializationError, BuiltinSerializationErrorKind, BuiltinTypeCheckError,
BuiltinTypeCheckErrorKind,
};

// Legacy migration types - to be removed when removing legacy framework
pub use scylla_cql::types::serialize::row::{
// Legacy migration types - to be removed when removing legacy framework
serialize_legacy_row,
ValueListAdapter,
ValueListToSerializeRowAdapterError,
};

// Not part of the old framework, but something that we should
// still aim to remove from public API.
pub use scylla_cql::types::serialize::row::{SerializedValues, SerializedValuesIterator};
}

/// Contains the [SerializeValue][value::SerializeValue] trait and its implementations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

pub mod value {
// Main types
pub use scylla_cql::types::serialize::value::SerializeValue;

// Errors
pub use scylla_cql::types::serialize::value::{
BuiltinSerializationError, BuiltinSerializationErrorKind, BuiltinTypeCheckError,
BuiltinTypeCheckErrorKind, MapSerializationErrorKind, MapTypeCheckErrorKind,
SetOrListSerializationErrorKind, SetOrListTypeCheckErrorKind,
TupleSerializationErrorKind, TupleTypeCheckErrorKind, UdtSerializationErrorKind,
UdtTypeCheckErrorKind,
};

// Legacy migration types - to be removed when removing legacy framework
pub use scylla_cql::types::serialize::value::{
serialize_legacy_value, ValueAdapter, ValueToSerializeValueAdapterError,
};
}

/// Contains types and traits used for safe serialization of values for a CQL statement.
pub mod writers {
pub use scylla_cql::types::serialize::writers::{
CellOverflowError, CellValueBuilder, CellWriter, RowWriter, WrittenCellProof,
};
}
}

/// Deserializing DB response containing CQL query results.
Expand Down
Loading