tikv / fail-rs

Fail points for rust
Apache License 2.0
332 stars 39 forks source link

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

Closed lucab closed 5 years ago

lucab commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

+1. negative features bad

brson commented 5 years ago

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