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

cargo feature should be opt-in, not opt-out #35

Closed
lucab opened this issue May 13, 2019 · 4 comments · Fixed by #37
Closed

cargo feature should be opt-in, not opt-out #35

lucab opened this issue May 13, 2019 · 4 comments · Fixed by #37

Comments

@lucab
Copy link
Member

lucab commented May 13, 2019

I have a usecase where I'd like to add failpoints across several of my libraries, however I'm experiencing some friction due to the way that cargo features are used by this crate.

Failpoints are currently active by default and needs to be disabled (opt-out) in production via the no_fail cargo feature. This poses a problem when nesting a couple of levels of dependencies, as the top-level consumer is no more in charge of those features and can't directly opt-out.

Considering that cargo features are additive, a better approach would be to make failpoints disabled by default and enabling them via a dedicated feature (opt-in). That way, the top-level application/consumer would be optionally in charge of configuring the fail environment and enabling failpoints (transparent to all intermediate libraries).

In practice, this would mean:

  • getting rid of the no_fail feature
  • making failpoints disabled by default
  • introducing a failpoints feature to enable them
  • releasing fail-0.3 with the new semantic

If this sounds fine to you, I can have a look around and send a PR in the next weeks.

/cc @BusyJay @kennytm @Hoverbear @brson

@BusyJay
Copy link
Member

BusyJay commented May 14, 2019

Make sense. I agree it's a better way giving that features are additive. Would you like to send us a pull request too? Although the feature name may need to be more expressive. It's confusing to have a features name the same as the crate.

@lucab
Copy link
Member Author

lucab commented May 14, 2019

@BusyJay yes, I'll be happy to put together a PR (with relaxed timing, i.e. whenever I have some time to look into it).
The crate is called fail and the macro fail_point!(). I think that failpoints is a reasonable feature name and doesn't collide with crate name nor macro name. I'm following https://rust-lang-nursery.github.io/api-guidelines/naming.html#feature-names-are-free-of-placeholder-words-c-feature here.
Final user-flow would be like this: cargo run --features failpoints

@brson
Copy link
Collaborator

brson commented Jun 5, 2019

+1. negative features bad

@brson
Copy link
Collaborator

brson commented Jun 5, 2019

@lucab I'd say go for the patch as you describe, and any objections can be brought up in review

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 a pull request may close this issue.

3 participants