Make ContextCompat consistent and non-ambiguous with WrapErr
#150
Conversation
ten3roberts
force-pushed
the
fix-ambiguous-methods
branch
from
fc96488
to
9f4cb62
Compare
LeoniePhiline
added
C-enhancement
thenorili
added
the
breaking change
There was a problem hiding this comment.
This looks like a good change to me, that ambiguity between option and result is detrimental and doesn't fit into our idea of what errors are.
It's a breaking change, so it'll need to wait for a major release.
Could you possibly write up a commit message to match? Other than that it looks fine to go on the experimental branch.
ten3roberts
force-pushed
the
fix-ambiguous-methods
branch
2 times, most recently
from
0cc07ae
to
e3dcdd2
Compare
| @@ -154,7 +158,7 @@ fn test_option_compat_wrap_err_with() { | |||
| })); | |||
|
|
|||
| use eyre::ContextCompat; | |||
| let err = None::<()>.wrap_err_with(|| "oopsie").unwrap_err(); | |||
| let err = None::<()>.with_context(|| "oopsie").unwrap_err(); | |||
There was a problem hiding this comment.
This correctly adheres to our model now as you can not "wrap an error" over an option.
You now either use the anyhow compatibility flag, or as the AnyhowCompat suggests the std methods like ok_or_else etc
ten3roberts
force-pushed
the
fix-ambiguous-methods
branch
from
8484124
to
8272695
Compare
|
@thenorili I've fixed up the commit message now and cleanup up the PR, let me know what you think 😊 |
There was a problem hiding this comment.
a couple typos, lgtm
The anyhow compatiblity methods (
contextandwith_context) where
introduced as conditional required methods for the traitWrapErr, as
well as another separate traitContextCompat, implemented for options
and results.
s/compatiblity/compatibility
s/where/were
ten3roberts
force-pushed
the
fix-ambiguous-methods
branch
from
8272695
to
6ebe72b
Compare
There was a problem hiding this comment.
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.
suggested:
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.
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"
s/no/not
suggested:
Furthermore, wrapping options doesn't align with our error model where errors are wrapped around other errors; an error wrapping "None" should just be a single error.
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
ten3roberts
force-pushed
the
fix-ambiguous-methods
branch
from
6ebe72b
to
a039517
Compare
|
Thanks, will merge the CI fix before to get this to pass |
There was a problem hiding this comment.
Noticed some inconsistency in trait use placement within tests, where use eyre::ContextCompat was moved upwards from the original use eyre::WrapErr placement, while other use eyre::ContextCompat remain placed right before trait method invocation.
(Let me know if this is too nitpicky <3)
| @@ -117,12 +118,13 @@ fn test_context() { | |||
| #[cfg(feature = "anyhow")] | |||
| #[test] | |||
| fn test_with_context() { | |||
| use eyre::ContextCompat; | |||
There was a problem hiding this comment.
As above:
For consistency with the other tests, this
usecould be moved to after the hook setup, just before usingContextCompat::with_context.Or the existing
uses moved to the start of each test fn as well.Examples of
useright before trait usage:
| @@ -100,12 +100,13 @@ fn test_option_ok_or_eyre() { | |||
| #[cfg(feature = "anyhow")] | |||
| #[test] | |||
| fn test_context() { | |||
| use eyre::ContextCompat; | |||
There was a problem hiding this comment.
For consistency with the other tests, this use could be moved to after the hook setup, just before using ContextCompat::with_context.
Or the existing uses moved to the start of each test fn as well.
Examples of use right before trait usage:
See: #149
Fixes: #149