eyre-rs / eyre

A trait object based error handling type for easy idiomatic error handling and reporting in Rust applications
Apache License 2.0
1.35k stars 63 forks source link

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

Closed LeoniePhiline closed 5 months ago

LeoniePhiline commented 7 months ago

1. Clarify trait usage in location test

8522f02

Both WrapErr and ContextCompat expose anyhow compatibility methods.

This has led to 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 (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.

2. Remove anyhow feature flag from OptionExt location test

68744f1

This bug was introduced (by me) in https://github.com/eyre-rs/eyre/commit/34bd1d98937d775dc8916a638d7ac0eece9e6e97#diff-1ff47dac6cf55e34ff587968c5b1f1ec6b6ae39d2668a66ecba3633163a21fc5R86 - partly due to the confusion fixed with the above commit.

Fixes https://github.com/eyre-rs/eyre/issues/147

LeoniePhiline commented 7 months ago

I don't think this requires a changelog entry.

ten3roberts commented 7 months ago

I see, good catch.

I recently learnt that conditional trait methods are a backwards compatability nightmare (though WrapErr is sealed thankfully), as there was a recent wasmtime discussion regarding failure to compile because a trait implemented new methods another crate didn't implement under a certain feature flag combiniation.

A better option would be to move these to a supertrait or ContextCompat, and make trait implement for all errors to expose the same behavior as present, without footguns.

LeoniePhiline commented 7 months ago

That would be an entirely different changeset, though, right?

ten3roberts commented 7 months ago

Yes, just a proposition, as it would alleviate the confusion with two identically named methods from two different traits.

From what I see in the current source code ContextCompat is only implemented for Option, while WrapErr has the context method for errors.

If you are willing to do this, or want me to do this, just let me know.

It is a larger change, but would alleviate some of the confusion and import confusion for downstream users

LeoniePhiline commented 7 months ago

My intention was to work on https://github.com/eyre-rs/eyre/issues/136 - I only discovered https://github.com/eyre-rs/eyre/issues/147 when starting to look into #136.

If you have time, I'd be glad if you could implement your proposition. I am not entirely sure if I fully see your vision in my mind.

ten3roberts commented 7 months ago

Got my idea here: https://github.com/eyre-rs/eyre/pull/150

I feel I need to clarify that this discussion is more about 8522f02 than the removal of the feature gate of the OptionExt trait.

Just doing what I can to clear out any confusion and make eyre as consistent as possible while we can.

In short, we move the context* methods to ContextCompat and implement it for Options and Results, to make all anyhow compatibility methods reside in one trait rather than two. This has the downside currently of wrap_err not being exposed for Options, but this just leans into the semantics of wrap an error with another higher level error, which does not make much sense for an Option.

When the anyhow feature drops from default features, errors and anyhow+options will be more self-contained and not lead to the same wrap_err in ContextCompat and WrapErr, and context from ContextCompat and WrapErr traits anymore.

Let me know what you think of this.

ten3roberts commented 5 months ago

Thank you for your work Leonie.

Sorry for leaving this hanging, had a rough time lately, but I am back now :tada:

LeoniePhiline commented 5 months ago

Please do not worry at all! Your wellbeing is always more important. <3