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

Use explicit features rather than implicit. #1473

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waywardmonkeys
Copy link
Contributor

@waywardmonkeys waywardmonkeys commented Jul 20, 2024

  • Added a CHANGELOG.md entry

Summary

In Rust 2024 edition, implicit features from optional dependencies are going away and explicit features are required.

Motivation

Preparing for the future, making features more clear when using cargo add.

Details

This changes all features using optional dependencies to use the "dep:" syntax to avoid creating implicit features.

It also adds new explicit features for optional dependencies that didn't previously have an explicit feature to enable them.

This does mean that some previously publicly visible features are now gone, like serde. These features would not have worked previously as the code is guarded by, for example, checks for feature = "serde1", so just enabling the optional dependency wouldn't have been sufficient to enable the code.

@dhardy
Copy link
Member

dhardy commented Jul 20, 2024

dep: is available from rust 1.60 and our MSRV is now 1.61, so that works.

It seems like every other crate offering a serde feature uses serde while we use serde1 (I had assumed that serde v2 might arrive before too long, but so far it has not). We will be making a breaking release soon; should we take the opportunity to standardise on serde?

@dhardy dhardy added the E-question Participation: opinions wanted label Jul 20, 2024
@waywardmonkeys
Copy link
Contributor Author

I think that would be good. I'd be happy to do that if there's agreement that it should happen. (I don't know how this project is governed.)

@vks
Copy link
Collaborator

vks commented Jul 24, 2024

IIRC, we were asked by someone (maybe @dtolnay?) back in the day to use serde1 instead of serde. I don't mind changing it (either way, we can introduce a serde2 feature in the future and keep backwards compatibility), but I'm not sure it's worth the churn.

@dhardy
Copy link
Member

dhardy commented Jul 24, 2024

IIRC it was my idea to use serde1. I don't care much; standardisation does make things more predictable however, saving new users from looking up documentation on expected features (serde being very widely adopted).

@waywardmonkeys
Copy link
Contributor Author

I've opened #1477 which does the rename, if that's the way you decide to go. I would then rebase this PR once it landed to pick up the remaining explicit dependencies.

@dhardy
Copy link
Member

dhardy commented Jul 24, 2024

Thanks. I'm really just waiting to see if other people have opinions on this.

@waywardmonkeys waywardmonkeys force-pushed the explicit-features branch 2 times, most recently from 6d4e29d to 718d7d4 Compare July 26, 2024 08:26
In Rust 2024 edition, implicit features from optional dependencies
are going away and explicit features are required.

This changes all features using optional dependencies to use the
"dep:" syntax to avoid creating implicit features.

It also adds new explicit features for optional dependencies that
didn't previously have an explicit feature to enable them.

One user-visible implicit feature has now been removed:
`rand` no longer has an implicit feature `rand_chacha`
as that is enabled by `std_rng`.
@waywardmonkeys
Copy link
Contributor Author

@dhardy This is now rebased on top of the rename of the serde feature. This means that almost all of the CHANGELOG entries were able to be removed and there's now only one user-visible change.

@@ -30,6 +30,7 @@ alloc = ["rand/alloc"]
std_math = ["num-traits/std"]

serde = ["dep:serde", "rand/serde"]
serde_with = ["dep:serde_with"]
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't show up either in API docs or the README. Could you add at least the latter please, preferably also a serialisation test?

Or we could just roll it under the serde feature. Either is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added in 0f5af66 (PR #1292) by @Armavica

Any chance they're still around and can comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was introduced for serializing const arrays, see the commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so it should be just part of the serde feature and not a separate (previously implicit) serde_with feature.

I'll update this PR over the weekend for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree!

@dhardy dhardy added D-changes Do: changes requested and removed E-question Participation: opinions wanted labels Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-changes Do: changes requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants