std::error::Error

[vbfox]

Motivation

Errors exposed are currently using the failure crate but recent Rust releases have stabilized std::error::Error, this PR is a proposal to change the errors to implement the standard trait using the derive implementation of the thiserror crate.

I also took the liberty to add a few more details to errors and refactor things to use map_error/ok_or and other functions a little more.

Test Plan

Ran cargo test

Discussion points

• I like the proposal of the thiserror crate because it's simply providing a derive and doesn't leak to consumers but If there are better ones...
• I can remove any of the "I too the liberty" part or split it in another PR at convenience, i'm just lazy and trying to avoid rebasing too often 😓
• And yes I know that i'm already creating conflicts with at least #24 and #19 🙄

[Mythra]

Hey,

Thanks for this PR! Unfortunately I don't see much benefit in the actual content of this PR. So I'm going to have to turn it down at least for the reasons provided. It is still possible to get: std::error::Error from a failure type with the compat method: https://docs.rs/failure/0.1.6/failure/struct.Error.html#method.compat . So it's not like this PR gives us something we can't do. It seems to be switching from one style to another just because one is preferred by you. If there was a compelling reason, that just wasn't mendioned I'd personally prefer anyhow, or waiting for: https://github.com/yaahc/eyre to become a bit more stabilized. (but I don't yet see a reason to switch over to those). I also am against changing everything to map_err/map_ok. This is again a style change without any particular benefit in runtime, maintainability, or build cost.

I'd like to re-iterate I really appreciate all the work you've been putting into this library, and hope that you will continue to as time goes on. But at the same time please do not file PRs that cannot point to concrete benefits or differences.

For example this PR was opened with the motivation:

Errors exposed are currently using the failure crate but recent Rust releases have stabilized std::error::Error, this PR is a proposal to change the errors to implement the standard trait using the derive implementation of the thiserror crate.

But this isn't actually a motivating factor. You're telling me what was done, but not why. Failure can already desugar to std::error::Error with the compat method. In the motivation you should explain what concrete benefit we are getting out of this. Just because there are two ways of doing something doesn't mean we should switch from one to the other if it doesn't get us anything. Just because someone says it's more "idiomatic" doesn't matter to me if it's not giving me a benefit in runtime, compile time, long term maintainability, or stability.

[Mythra]

I should also mention even if this PR is closed, and we decide there is no benefit in say switching to anyhow/eyre, if you'd like to file a PR changing the error messages to your more helpful variants I would welcome that. Better error messages for developers are a tangible benefit, and would be welcome.

[vbfox]

I think we'll have to agree to disagree in the end because I disagree with quite a bit:

• From what I see, failure is mostly in pure maintenance mode following std::error::Error latest changes
• Using failure compatibility mode only generate only the Display implementation, source is completely lost there is no good way to get it back. So if I PR my changes to provide better error messages I lose access to the source error unless I add that info back manually
• Non-enum errors like failure generic errors or anyhow can't be matched by consumer code as easily and it don't solve the adding error message problems. I suspect that's why anyhow author also make thiserorr and suggest one for apps and the other for libraries https://github.com/dtolnay/anyhow#comparison-to-thiserror
• On readability of manual error handling too I don't really agree, I have a lot of difficulty finding what the code is doing on the left in the middle of all the error handling versus the code on the right: One less dramatic change that I find help readability is this one : In both cases I it find both easier to read what the code is trying to do and to review that errors are correctly handled (because it's on the same line, not later and the only possible outcome that continue execution is a success)

[Mythra]

You did actually outline one benefit here I agree with, Unfortunately this PR did not outline it. Specifically:

• Using failure compatibility mode only generate only the Display implementation, source is completely lost there is no good way to get it back. So if I PR my changes to provide better error messages I lose access to the source error unless I add that info back manually -> losing traits on moving to std::Error.

That is a tangible thing a user can get. A user can get a backtrace when using standard error. I still don't particularly like thiserrors' implementation, but I'd be open to other ideas.

• From what I see, failure is mostly in pure maintenance mode following std::error::Error latest changes -> is it actually marked as deprecated? There seems to be no mention of it going away: https://github.com/rust-lang-nursery/failure

• On readability of manual error handling too I don't really agree, I have a lot of difficulty finding what the code is doing on the left in the middle of all the error handling versus the code on the right:. Admitedlly this code does look much nicer but it should be it's own PR. It did not seem nicer in Github's Diff view.