Make `ContextCompat` consistent and non-ambiguous with `WrapErr` #150

ten3roberts · 2024-01-24T16:35:31Z

Fixes: #149

thenorili

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 · 2024-03-12T21:14:08Z

eyre/tests/test_location.rs

@@ -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();


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 · 2024-03-12T21:26:39Z

@thenorili I've fixed up the commit message now and cleanup up the PR, let me know what you think 😊

thenorili

a couple typos, lgtm

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.

s/compatiblity/compatibility
s/where/were

thenorili

This led to confusion as the two distinct trait
s/trait/traits

thenorili

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 · 2024-03-28T21:01:14Z

Thanks, will merge the CI fix before to get this to pass

LeoniePhiline

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)

LeoniePhiline · 2024-03-31T12:36:49Z

eyre/tests/test_location.rs

@@ -117,12 +118,13 @@ fn test_context() {
 #[cfg(feature = "anyhow")]
 #[test]
 fn test_with_context() {
+    use eyre::ContextCompat;


As above:

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:

https://github.com/eyre-rs/eyre/pull/150/files#diff-1ff47dac6cf55e34ff587968c5b1f1ec6b6ae39d2668a66ecba3633163a21fc5L142-R145

https://github.com/eyre-rs/eyre/pull/150/files#diff-1ff47dac6cf55e34ff587968c5b1f1ec6b6ae39d2668a66ecba3633163a21fc5L157-R160

LeoniePhiline · 2024-03-31T12:39:52Z

eyre/tests/test_location.rs

@@ -100,12 +100,13 @@ fn test_option_ok_or_eyre() {
 #[cfg(feature = "anyhow")]
 #[test]
 fn test_context() {
+    use eyre::ContextCompat;


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:

https://github.com/eyre-rs/eyre/pull/150/files#diff-1ff47dac6cf55e34ff587968c5b1f1ec6b6ae39d2668a66ecba3633163a21fc5L142-R145

https://github.com/eyre-rs/eyre/pull/150/files#diff-1ff47dac6cf55e34ff587968c5b1f1ec6b6ae39d2668a66ecba3633163a21fc5L157-R160

This filters out diverging backtraces caused by different before-main and test calling machinery on windows and linux

thenorili

LGTM

thenorili · 2024-06-27T04:29:22Z

eyre/src/lib.rs

@@ -643,6 +645,7 @@ impl dyn EyreHandler {
        }
    }

+    /// Downcast the handler to a contcrete type `T`


nit "concrete"

LeoniePhiline · 2024-06-28T11:22:54Z

eyre/tests/test_location.rs

@@ -140,7 +142,7 @@ fn test_option_compat_wrap_err() {
    }));

    use eyre::ContextCompat;
-    let err = None::<()>.wrap_err("oopsie").unwrap_err();
+    let err = None::<()>.context("oopsie").unwrap_err();


The encosing test test_option_compat_wrap_err might better be renamed to test_option_compat_context?

test_option_compat_context and test_option_compat_with_context already exist and are (now) identical to test_option_compat_wrap_err and test_option_compat_wrap_err_with.

The tests test_option_compat_wrap_err and test_option_compat_wrap_err_with are obsolete and should be deleted.

And this is why consistent naming is so good, cause now that yields a duplicate definition error.

Thanks a lot for your suggestions 😊

LeoniePhiline · 2024-06-28T11:23:10Z

eyre/tests/test_location.rs

@@ -155,7 +157,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();


The encosing test test_option_compat_wrap_err_with might better be renamed to test_option_compat_with_context?

Fixes eyre-rs#150 (comment)

Fixes eyre-rs#150 (comment), eyre-rs#150 (comment)

LeoniePhiline · 2024-06-28T11:52:45Z

#179 should fix my comments.

SuperFluffy · 2024-09-17T13:48:30Z

This caught me today. I would love to see this released soon. 🙏

ten3roberts mentioned this pull request Jan 24, 2024

fix: remove anyhow feature flag from OptionExt location test #148

Merged

ten3roberts force-pushed the fix-ambiguous-methods branch from fc96488 to 9f4cb62 Compare January 24, 2024 16:55

LeoniePhiline added C-enhancement Category: New feature or request A-eyre Area: eyre subcrate labels Feb 20, 2024

thenorili added the breaking change Non-urgent breaking changes, probably delay to the next release label Feb 29, 2024

thenorili requested changes Feb 29, 2024

View reviewed changes

ten3roberts force-pushed the fix-ambiguous-methods branch 2 times, most recently from 0cc07ae to e3dcdd2 Compare March 12, 2024 21:04

ten3roberts commented Mar 12, 2024

View reviewed changes

ten3roberts force-pushed the fix-ambiguous-methods branch from 8484124 to 8272695 Compare March 12, 2024 21:20

ten3roberts requested a review from thenorili March 15, 2024 12:27

ten3roberts marked this pull request as ready for review March 15, 2024 12:33

thenorili approved these changes Mar 28, 2024

View reviewed changes

ten3roberts force-pushed the fix-ambiguous-methods branch from 8272695 to 6ebe72b Compare March 28, 2024 20:14

thenorili reviewed Mar 28, 2024

View reviewed changes

thenorili requested changes Mar 28, 2024

View reviewed changes

ten3roberts force-pushed the fix-ambiguous-methods branch from 6ebe72b to a039517 Compare March 28, 2024 20:37

thenorili approved these changes Mar 28, 2024

View reviewed changes

Merge branch 'master' into fix-ambiguous-methods

2b32acd

ten3roberts mentioned this pull request Mar 29, 2024

Fix ci breaking due to new dependencies and rustc changes (backport to 0.6) #162

Merged

fix: warnings from new clippy lints

90645ef

ten3roberts mentioned this pull request Mar 29, 2024

Fix ci breaking due to new dependencies and rustc changes (backport to master) #163

Merged

LeoniePhiline reviewed Mar 31, 2024

View reviewed changes

ten3roberts added 2 commits June 21, 2024 19:48

Merge branch 'master' into fix-ambiguous-methods

39ec865

fix: different call machinery between linux and windows

4d6e73e

This filters out diverging backtraces caused by different before-main and test calling machinery on windows and linux

thenorili approved these changes Jun 27, 2024

View reviewed changes

LeoniePhiline reviewed Jun 28, 2024

View reviewed changes

LeoniePhiline added a commit to LeoniePhiline/eyre that referenced this pull request Jun 28, 2024

style: consistently place use eyre::ContextCompat in location test

f9f3a35

Fixes eyre-rs#150 (comment)

LeoniePhiline added a commit to LeoniePhiline/eyre that referenced this pull request Jun 28, 2024

chore: remove obsolete impl ContextCompat for Option location tests

43b9192

Fixes eyre-rs#150 (comment), eyre-rs#150 (comment)

LeoniePhiline mentioned this pull request Jun 28, 2024

Fix ambiguous methods #179

Closed

ten3roberts added 2 commits June 28, 2024 17:35

fix: typo in doc comment

231090d

fix: import consistency

0eabf81

ten3roberts force-pushed the fix-ambiguous-methods branch from db04328 to 0eabf81 Compare June 28, 2024 15:35

fix: consistent naming and duplicated tests

c98bc8c

ten3roberts merged commit 9498677 into master Jun 28, 2024
29 checks passed

ten3roberts deleted the fix-ambiguous-methods branch June 28, 2024 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `ContextCompat` consistent and non-ambiguous with `WrapErr` #150

Make `ContextCompat` consistent and non-ambiguous with `WrapErr` #150

ten3roberts commented Jan 24, 2024 •

edited

Loading

thenorili left a comment

ten3roberts Mar 12, 2024

ten3roberts commented Mar 12, 2024 •

edited

Loading

thenorili left a comment

thenorili left a comment

thenorili left a comment

ten3roberts commented Mar 28, 2024

LeoniePhiline left a comment •

edited

Loading

LeoniePhiline Mar 31, 2024

LeoniePhiline Mar 31, 2024

thenorili left a comment

thenorili Jun 27, 2024

LeoniePhiline Jun 28, 2024

LeoniePhiline Jun 28, 2024 •

edited

Loading

ten3roberts Jun 28, 2024

LeoniePhiline Jun 28, 2024

LeoniePhiline commented Jun 28, 2024

SuperFluffy commented Sep 17, 2024

Make ContextCompat consistent and non-ambiguous with WrapErr #150

Make ContextCompat consistent and non-ambiguous with WrapErr #150

Conversation

ten3roberts commented Jan 24, 2024 • edited Loading

thenorili left a comment

Choose a reason for hiding this comment

ten3roberts Mar 12, 2024

Choose a reason for hiding this comment

ten3roberts commented Mar 12, 2024 • edited Loading

thenorili left a comment

Choose a reason for hiding this comment

thenorili left a comment

Choose a reason for hiding this comment

thenorili left a comment

Choose a reason for hiding this comment

ten3roberts commented Mar 28, 2024

LeoniePhiline left a comment • edited Loading

Choose a reason for hiding this comment

LeoniePhiline Mar 31, 2024

Choose a reason for hiding this comment

LeoniePhiline Mar 31, 2024

Choose a reason for hiding this comment

thenorili left a comment

Choose a reason for hiding this comment

thenorili Jun 27, 2024

Choose a reason for hiding this comment

LeoniePhiline Jun 28, 2024

Choose a reason for hiding this comment

LeoniePhiline Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

ten3roberts Jun 28, 2024

Choose a reason for hiding this comment

LeoniePhiline Jun 28, 2024

Choose a reason for hiding this comment

LeoniePhiline commented Jun 28, 2024

SuperFluffy commented Sep 17, 2024

Make `ContextCompat` consistent and non-ambiguous with `WrapErr` #150

Make `ContextCompat` consistent and non-ambiguous with `WrapErr` #150

ten3roberts commented Jan 24, 2024 •

edited

Loading

ten3roberts commented Mar 12, 2024 •

edited

Loading

LeoniePhiline left a comment •

edited

Loading

LeoniePhiline Jun 28, 2024 •

edited

Loading