Skip to content

Put everything behind a 'stable' feature to avoid future breaking changes #434

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

Merged
merged 3 commits into from
Nov 2, 2019
Merged

Conversation

paulocsanz
Copy link
Contributor

@paulocsanz paulocsanz commented Nov 1, 2019

This fixes #417, although I don't know if this is the ideal way, can anybody point me the best way?

I had to define the macro in lib.rs otherwise everything else on util.rs would have to be behind the same macro, I could move it to utils.rs tho.

Also, since things on unstable seem to depend on stable (does it really need to?). So for now unstable needs stable, that could become a problem when specific features are added, since you may need to play feature hide and seek with them to use the unstable features. What do you guys think about that?

If we put only the pub exports behind the flag a bunch of "unused" warnings will pop up, is it better than putting everything behind the flag (it may fix the unstable problem)? Should we just add a allow(unused)?

And for some weird reason prelude.rs stopped compiling referencing this PR rust-lang/rust#52234, which seems like a problem so I removed the pub use, specially because now the macro won't be in the prelude. Any ideas on that? (this actually breaks a test, what is the best way to fix it?)

src/lib.rs Outdated
@@ -49,33 +49,46 @@
#![doc(html_logo_url = "https://async.rs/images/logo--hero.svg")]
#![recursion_limit = "2048"]

#[macro_use]
mod utils;
/// Declares stable items.
Copy link

Choose a reason for hiding this comment

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

Can you move this macro into utils.rs to group all these macros together?

Copy link

Choose a reason for hiding this comment

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

Or, alternatively, I believe you can just put this in lib.rs :)

#![cfg(feature = "stable")]

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Nov 1, 2019

I think this idea is worthwhile for us to introduce other features down the line (such as a subset of async-std that does not build the runtime).

I'm not sure what the right name is, but I like the name default, as the only purpose for it is to be disabled through no-default, and then re-enable other features -- possibly runtime and core.

@ghost
Copy link

ghost commented Nov 1, 2019

@yoshuawuyts I like that idea!

I believe this whole PR can be reduced to (seems to work for me!):

  1. Add default = [] to Cargo.toml.
  2. Add #![cfg(feature = "default")] to lib.rs.

If we add new features like runtime or core, we'd change that attribute to:

#![cfg(any(feature = "default", feature = "runtime", feature = "core"))]

@paulocsanz
Copy link
Contributor Author

@stjepang @yoshuawuyts How about now?

@ghost
Copy link

ghost commented Nov 1, 2019

Looks good to me but I've never seen a crate do this before. Do we know someone more familiar with Cargo who could take a quick look?

@taiki-e
Copy link
Contributor

taiki-e commented Nov 1, 2019

I recently confirmed this behavior when working on cargo-hack: https://github.com/taiki-e/cargo-hack/blob/75bff8d3c45e72e2075daf998b7dd179c8f7848c/src/main.rs#L243-L246

    // run with no default features if the package has other features
    //
    // `default` is not skipped because `cfg(feature = "default")` is work
    // if `default` feature specified.

EDIT: However, I have never seen a crate doing this other than a crate for testing :)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I'm approving but it would be good for someone else to also approve before merging.

@yoshuawuyts yoshuawuyts merged commit 911ebad into async-rs:master Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create default stable feature before 1.0 to avoid future breaking changes
3 participants