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

Conversation

Lorak-mmk
Copy link
Collaborator

@Lorak-mmk Lorak-mmk commented Oct 14, 2024

It is easier to see what is imported, and decide if it should be, if one can see what exactly is imported. This is hard to do with stars, so, as part of API stabilization effort, those are removed here.

This PR also removes raw_batch re-export. This module should not be needed by users.

I verified using semver-checks that the first commit doesn't introduce any breaking changes.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

Copy link

github-actions bot commented Oct 14, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: dcd1790

@Lorak-mmk Lorak-mmk force-pushed the serialization_exports branch from fab9fe1 to 31c5dc2 Compare October 14, 2024 16:46
scylla/src/lib.rs Outdated Show resolved Hide resolved
@Lorak-mmk Lorak-mmk force-pushed the serialization_exports branch from fa3c5aa to ed05c32 Compare October 14, 2024 18:32
scylla/src/lib.rs Show resolved Hide resolved
@Lorak-mmk Lorak-mmk force-pushed the serialization_exports branch from ed05c32 to 72d29ba Compare October 15, 2024 10:28
@Lorak-mmk Lorak-mmk requested a review from muzarski October 15, 2024 10:29
It is easier to see what is imported, and decide if it should be,
if one can see what exactly is imported. This is hard to do with stars,
so, as part of API stabilization effort, those are removed here.

For now, I did not remove any imports, so this change should not
be breaking.
There should be no need for users to use this - it is only used
for interaction between scylla and scylla-cql.
@Lorak-mmk Lorak-mmk force-pushed the serialization_exports branch from 72d29ba to f649822 Compare October 15, 2024 11:05
@wprzytula wprzytula added the API-stability Part of the effort to stabilize the API label Oct 15, 2024
Comment on lines +138 to +139
/// Contains the [BatchValues][batch::BatchValues] and [BatchValuesIterator][batch::BatchValuesIterator] trait and their
/// implementations.
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.

@muzarski muzarski self-requested a review October 15, 2024 14:37
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

};
}

/// Contains the [RawBatchValues][raw_batch::RawBatchValues] and [RawBatchValuesIterator][raw_batch::RawBatchValuesIterator]
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

};
}

/// 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 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

@Lorak-mmk Lorak-mmk merged commit f9a3635 into scylladb:main Oct 16, 2024
11 checks passed
@Lorak-mmk Lorak-mmk deleted the serialization_exports branch October 16, 2024 15:51
@wprzytula wprzytula mentioned this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-stability Part of the effort to stabilize the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants