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.

Sign up for GitHub

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

Jump to bottom

[ BREAKING ] ContextCompat contains identically named methods to WrapErr which can be confusing and a footgun #149

Closed
ten3roberts opened this issue Jan 24, 2024 · 1 comment · Fixed by #150
Closed

[ BREAKING ] ContextCompat contains identically named methods to WrapErr which can be confusing and a footgun #149

ten3roberts opened this issue Jan 24, 2024 · 1 comment · Fixed by #150
Assignees
@ten3roberts
Labels
breaking change Non-urgent breaking changes, probably delay to the next release C-enhancement Category: New feature or request

Comments

@ten3roberts
Copy link
Contributor

This can be confusing as you can't tell which method is being called.

It also leads to a backwards compatability hazard for conditional trait methods, though these are currently alleviated by virtue of the trait being sealed.

Additionally, WrapErr also exposes anyhow compatibility methods like context, which leads to the same confusion and footguns in the other direction.

This has lead to misunderstandings, poor reasonability, as well as mistakes in code where we called the wrong method.

Consider making anyhow compatibility consistent under one trait.
@ten3roberts ten3roberts added the breaking change Non-urgent breaking changes, probably delay to the next release label Jan 24, 2024
@ten3roberts
Copy link
Contributor Author

Related: #147 and #138 (comment)

ten3roberts added a commit that referenced this issue Jan 24, 2024
@ten3roberts
fix: make ContextCompat consistent and non-ambiguous with WrapErr
fc96488 
See: #149
@ten3roberts ten3roberts mentioned this issue Jan 24, 2024
Merged
ten3roberts added a commit that referenced this issue Jan 24, 2024
@ten3roberts
fix: make ContextCompat consistent and non-ambiguous with WrapErr
9f4cb62 
See: #149
@LeoniePhiline LeoniePhiline mentioned this issue Feb 12, 2024
Open
@LeoniePhiline LeoniePhiline mentioned this issue Feb 20, 2024
Merged
ten3roberts added a commit that referenced this issue Mar 12, 2024
@ten3roberts
fix: remove ambiguity between options and results trait methods
0cc07ae 
1. Remove the conditional trait methods `context` and `with_context`
   from `WrapErr`
2. Remove ambiguity between "wrapping errors" with another, and
   converting a `None` value to an error.

The anyhow compatiblity methods (`context` and `with_context`) where
introduced as conditional required methods for the trait `WrapErr`, as
well as another separate trait `ContextCompat`, implemented for options.

The latter trait also provided methods `wrap_err` and `wrap_err_with`.
This led to confusion as the two distinct trait had the same methods,
except one was implemented on results, and one of options.

Further, this did not align with our error model, wherein errors are
wrapped with other errors in a chain now that options (which are no
errors and have no message) could be "wrapped"

See: #149
ten3roberts added a commit that referenced this issue Mar 12, 2024
@ten3roberts
fix: remove ambiguity between options and results trait methods
e3dcdd2 
1. Remove the conditional trait methods `context` and `with_context`
   from `WrapErr`
2. Remove ambiguity between "wrapping errors" with another, and
   converting a `None` value to an error.

The anyhow compatiblity methods (`context` and `with_context`) where
introduced as conditional required methods for the trait `WrapErr`, as
well as another separate trait `ContextCompat`, implemented for options
*and* results.

The latter trait also provided methods `wrap_err` and `wrap_err_with`.
This led to confusion as the two distinct trait had the same methods,
except one was implemented on results, and one of options and results.

Further, this did not align with our error model, wherein errors are
wrapped with other errors in a chain now that options (which are no
errors and have no message) could be "wrapped"

See: #149
ten3roberts added a commit that referenced this issue Mar 12, 2024
@ten3roberts
fix: remove ambiguity between options and results trait methods
8272695 
1. Remove the conditional trait methods `context` and `with_context`
   from `WrapErr`
2. Remove ambiguity between "wrapping errors" with another, and
   converting a `None` value to an error.

The anyhow compatiblity methods (`context` and `with_context`) where
introduced as conditional required methods for the trait `WrapErr`, as
well as another separate trait `ContextCompat`, implemented for options
*and* results.

The latter trait also provided methods `wrap_err` and `wrap_err_with`.
This led to confusion as the two distinct trait had the same methods,
except one was implemented on results, and one of options and results.

Further, this did not align with our error model, wherein errors are
wrapped with other errors in a chain now that options (which are no
errors and have no message) could be "wrapped"

See: #149
ten3roberts pushed a commit that referenced this issue Mar 14, 2024
@LeoniePhiline
fix: remove anyhow feature flag from OptionExt location test (#148)
75beaae 
# 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
@ten3roberts ten3roberts added the C-enhancement Category: New feature or request label Mar 15, 2024
@ten3roberts ten3roberts assigned ten3roberts and unassigned thenorili Mar 15, 2024
ten3roberts added a commit that referenced this issue Mar 28, 2024
@ten3roberts
fix: remove ambiguity between options and results trait methods
6ebe72b 
1. Remove the conditional trait methods `context` and `with_context`
   from `WrapErr`
2. Remove ambiguity between "wrapping errors" with another, and
   converting a `None` value to an error.

The anyhow compatibility methods (`context` and `with_context`) were
introduced as conditional required methods for the trait `WrapErr`, as
well as another separate trait `ContextCompat`, implemented for options
*and* results.

The latter trait also provided methods `wrap_err` and `wrap_err_with`.
This led to confusion as the two distinct trait had the same methods,
except one was implemented on results, and one of options and results.

Further, this did not align with our error model, wherein errors are
wrapped with other errors in a chain now that options (which are no
errors and have no message) could be "wrapped"

See: #149
ten3roberts added a commit that referenced this issue Mar 28, 2024
@ten3roberts
fix: remove ambiguity between options and results trait methods
a039517 
1. Remove the conditional trait methods `context` and `with_context`
   from `WrapErr`
2. Remove ambiguity between "wrapping errors" with another, and
   converting a `None` value to an error.

The anyhow compatibility methods (`context` and `with_context`) were
introduced as conditional required methods for the trait `WrapErr`, as
well as another separate trait `ContextCompat`, implemented for options
*and* results.

The latter trait also provided methods `wrap_err` and `wrap_err_with`.
This led to confusion as the two distinct traits had the same methods,
one was implemented on results, and the other on both options and
results.

Furthermore, wrapping options does not align with our error model where
errors are wrapped around other errors; an error wrapping `None` should
just be a single error and should not be wrapped.

See: #149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ten3roberts ten3roberts

Labels
breaking change Non-urgent breaking changes, probably delay to the next release C-enhancement Category: New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make ContextCompat consistent and non-ambiguous with WrapErr eyre-rs/eyre
2 participants
@ten3roberts @thenorili