Replace deprecated 'error-chain' with 'thiserror'#1820
Conversation
f85419f to
571e9f7
Compare
The reason is simply that String does not implement Error, and that would not be a good idea. See e.g.: https://internals.rust-lang.org/t/impl-error-for-string/8881
|
I was never really happy with this My understanding is that We're using I was quite happy with both crates so far. |
| ParseIntError(::std::num::ParseIntError); | ||
| GlobParsingError(::globset::Error); | ||
| SerdeYamlError(::serde_yaml::Error); | ||
| #[derive(Error, Debug)] |
There was a problem hiding this comment.
I don't understand what clippys problem is here...
There was a problem hiding this comment.
Most likely caused by thiserror allowing some clippy problems that our MSRV does not know about: https://github.com/dtolnay/thiserror/blob/master/impl/src/expand.rs#L120
Best way to fix is probably to --allow clippy::unknown_clippy_lints on MSRV clippy
There was a problem hiding this comment.
...or make use of clippy::msrv (thanks for the ping)
There was a problem hiding this comment.
I ended up using --allow clippy::unknown_clippy_lints since starting to use clippy::msrv is not trivial (requires moving to some other CI job)
|
I've now fixed remaining open issue and done some additional verification, and the code seems to work as it should. So feel free to take another look. If it looks good, we can (squash) merge this, as far as I'm concerned. |
We currently depend on error-chain. But
error-chainhas been deprecated:So I think getting rid of
error-chainwill not be controversial. Exactly HOW to get rid oferror-chainmight be a bit controversial though. To get the discussion started, I here have a proposal on how to do it, which uses thiserror.I did consider using
anyhow, but read somewhere that someone thought that libraries should not use any particular high-level error handling library, but instead let apps pick one, and I thought that made sense.thiserroris just a derive-macro so one does not have to implementErrortraits manually.I have not done broad verification yet, but all regression tests pass, and basic error reporting work as before. Examples: