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

feat!: introduce an "anyhow" compatibility layer feature flag #138

Conversation

LeoniePhiline
Copy link
Contributor

This change hides the anyhow compatibility layer behind an "anyhow" feature flag.
In eyre v0.6 the feature is enabled by default.

Fixes #131

eyre/src/context.rs Outdated Show resolved Hide resolved
eyre/src/error.rs Show resolved Hide resolved
eyre/src/lib.rs Show resolved Hide resolved
eyre/tests/test_location.rs Show resolved Hide resolved
This change hides the `anyhow` compatibility layer
behind an `"anyhow"` feature flag.
In `eyre` v0.6 the feature is enabled by default.

Fixes eyre-rs#131
@LeoniePhiline LeoniePhiline force-pushed the feat/131-hide-anyhow-compat-feature-flag branch from 1bb6956 to 3237c38 Compare December 20, 2023 16:02
Copy link
Contributor

@ten3roberts ten3roberts left a comment

Choose a reason for hiding this comment

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

Awesome!

@yaahc is there any function, trait, etc here that you see which shouldn't be behind this flag, or does it all look good?

@Nemo157
Copy link

Nemo157 commented Dec 28, 2023

Note, this is a breaking change, anyone using default-features = false and the compat layer will be broken.

@thenorili thenorili added the breaking change Non-urgent breaking changes, probably delay to the next release label Dec 30, 2023
@ten3roberts ten3roberts self-requested a review January 4, 2024 21:33
Copy link
Contributor

@ten3roberts ten3roberts left a comment

Choose a reason for hiding this comment

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

There is only one way around this breaking change.

Making the feature additive but negative, I.e no-anyhow-compat would work, but will break dependencies with the feature resolver.

As such, the only viable option would be to keep this as a positive feature flag, and improve it in the next release. Potentially, we can default this to off but this is yet to be decided.

To merge this now we could create a breaking changes branch and merge it into there, and then backport bugfixes to the current 0.6 release on main

@LeoniePhiline
Copy link
Contributor Author

Merging this on a new breaking changes branch sounds good! (Maybe better: using main as breaking changes branch and starting a 0.6 maintenance branch?)

Do I understand correctly that there is nothing to do from my side right now?

@ten3roberts
Copy link
Contributor

Certainly, a maintenance would work and we can backport all bug fixes from main to 0.6 to prevent them being forgotten.

That is correct, there is nothing that would need to be done on your part and the feature flag is really good 😊. I spoke to Jane and we didn't find that there was any item that we missed to include under the feature flag.

Copy link
Contributor

@ten3roberts ten3roberts left a comment

Choose a reason for hiding this comment

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

Awesome.

These changes will now land on master, which prepares for the next breaking release.

As Nemo pointed out, this is because disabling behavior by moving it to a default feature is a breaking change.

@ten3roberts ten3roberts merged commit 34bd1d9 into eyre-rs:master Jan 16, 2024
29 checks passed
@LeoniePhiline
Copy link
Contributor Author

Since it's a breaking change anyway, shall we then remove it from default features right away?

Otherwise we need yet another breaking release if we are ever going to make the anyhow compatibility layer opt-in only.

@ten3roberts
Copy link
Contributor

Absolutely, that would be great.

It would further go into our intents of making the error wrapping represent a logical error chain, rather than context attached to an error. The anyhow compatibility has unfortunately incentivized the latter approach where through API naming the error chains are treated like context stores where you can interpolate arbitrary data, such as usernames or other variables.

Making it opt-in would thus be awesome, and push towards a more descriptive chain of errors.

Do you mind opening a PR with a short explanation in the Readme of what it means and how to opt in/how it compares to anyhow?

@LeoniePhiline
Copy link
Contributor Author

LeoniePhiline commented Jan 23, 2024

Yes, I can do that! I added a note to #136 which already tracks that change.

@LeoniePhiline LeoniePhiline changed the title feat: introduce an "anyhow" compatibility layer feature flag feat!: introduce an "anyhow" compatibility layer feature flag Jan 23, 2024
LeoniePhiline added a commit to LeoniePhiline/eyre that referenced this pull request Jan 24, 2024
Both `WrapErr` and `ContextCompat` expose
`anyhow` compatibility methods.

`ContextCompat` is completely feature gated
behind the `anyhow` flag.

`WrapErr`, on the other hand, feature-gates
individual functions behind the `anyhow` flag.

This has led to [confusion][confusion]
in the past.

[confusion]: eyre-rs#138 (comment)
thenorili pushed a commit to LeoniePhiline/eyre that referenced this pull request Feb 29, 2024
Both `WrapErr` and `ContextCompat` expose
`anyhow` compatibility methods.

`ContextCompat` is completely feature gated
behind the `anyhow` flag.

`WrapErr`, on the other hand, feature-gates
individual functions behind the `anyhow` flag.

This has led to [confusion][confusion]
in the past.

[confusion]: eyre-rs#138 (comment)
ten3roberts pushed a commit that referenced this pull request Mar 14, 2024
# 1. Clarify trait usage in location test


[`8522f02`](8522f02)

Both `WrapErr` and `ContextCompat` expose `anyhow` compatibility
methods.

- `ContextCompat` is completely feature gated behind the `anyhow` flag.

- `WrapErr`, on the other hand, feature-gates individual functions
behind the `anyhow` flag.

This has led to [confusion][confusion] in the past.

This change moves the `use eyre::WrapErr` statement from the top of the
module into each individual test, as [identically named
functions](#149) (`wrap_err.*`)
are exposed both by `WrapErr` and `ContextCompat`.

With `use eyre::WrapErr` moved directly into the tests, it becomes clear
which trait-provided `fn` is being called.

[confusion]:
#138 (comment)

# 2. Remove `anyhow` feature flag from `OptionExt` location test 


[`68744f1`](68744f1)

This bug was introduced (by me) in
34bd1d9#diff-1ff47dac6cf55e34ff587968c5b1f1ec6b6ae39d2668a66ecba3633163a21fc5R86
- partly due to the confusion fixed with the above commit.

Fixes #147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Non-urgent breaking changes, probably delay to the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ergonomics: Hide context behind a feature
4 participants