Change error handling from error-chain to anyhow.#777
Change error handling from error-chain to anyhow.#777nnethercote merged 1 commit intomozilla:masterfrom
error-chain to anyhow.#777Conversation
|
Apologies for the large review. I haven't run rustfmt over this, so it should wait until #776 has landed and then I can rebase and reformat. |
|
I did this same conversion in our work codebase a while ago and have been happy with the outcome, FWIW. I also pulled in Not that my opinion matters these days but I wouldn't say I love the One other thing I found when doing this in our work codebase was that |
I considered it but was convinced by this comment not to use it. Doing it by hand for the three internal errors was easy.
Early on in the conversion I changed
Does that mean I can change this: to this: ? That would be nice, there are plenty of |
|
@froydnj: I have rebased, addressed Ted's comments, and run rustfmt. It's ready for you to take a look, thanks. BTW, here is the current diffstats. Not bad:
|
Because `anyhow` is what seems to be the most common error library for
applications these days.
- The global `Result` type is now `anyhow::Result`.
- In errors.rs, there's no need for any boilerplate to wrap all the foreign
errors seen: `hyper::Error`, `io:Error`, etc.
- The internal errors that we care about are now separate types, rather
than within an enum, because that works better when we need to check for them
by downcasting an `anyhow::Error`. And it's nice to write
`Err(ProcessError(output))` rather than
`Err(ErrorKind::ProcessError(output))`.
- The `Which` error was unused and is removed.
- The most common change is that `.chain_err()` is changed to
`.context`/`.with_context()`.
- `anyhow!` is used where necessary, mostly to promote a string to an
`anyhow::Error`.
- Errors within futures: `FutureChainErr`/`.chain_err()` is changed to
`FutureContext`/`fcontext`/`fwith_context`. The `f` prefix is because I found
it helpful to distinguish these cases from the simple error cases.
- `BuilderIncoming`, `SchedulerIncoming`, `ServerIncoming` no longer have an
`Error` associated type, we just use `anyhow::Error` uniformly.
- `e.display_chain()` changes to `format!("{:?}")`, because they both print the
full cause chain, and the backtrace (if present).
- A few places where the old code was doing something weird or more
complicated than seemed necessary, I generally tried to replace it with
something simpler and more typical. Two places used `with_boxed_chain()`,
which doesn't have an equivalent in `anyhow`, so I did my best to do
something reasonable.
- In `src/server.rs` we now import `std::task::Context` as `TaskContext` to
avoid overshadowing the `anyhow::Context` trait :(
froydnj
left a comment
froydnj
left a comment
There was a problem hiding this comment.
Thank you, this is much nicer.
3 participants
Because
anyhowis what seems to be the most common error library forapplications these days.
The global
Resulttype is nowanyhow::Result.In errors.rs, there's no need for any boilerplate to wrap all the foreign
errors seen:
hyper::Error,io:Error, etc.The internal errors that we care about are now separate types, rather
than within an enum, because that works better when we need to check for them
by downcasting an
anyhow::Error. And it's nice to writeErr(ProcessError(output))rather thanErr(ErrorKind::ProcessError(output)).The
Whicherror was unused and is removed.The most common change is that
.chain_err()is changed to.context/.with_context().anyhow!is used where necessary, mostly to promote a string to ananyhow::Error.Errors within futures:
FutureChainErr/.chain_err()is changed toFutureContext/fcontext/fwith_context. Thefprefix is because I foundit helpful to distinguish these cases from the simple error cases.
BuilderIncoming,SchedulerIncoming,ServerIncomingno longer have anErrorassociated type, we just useanyhow::Erroruniformly.e.display_chain()changes toformat!("{:?}"), because they both print thefull cause chain, and the backtrace (if present).
A few places where the old code was doing something weird or more
complicated than seemed necessary, I generally tried to replace it with
something simpler and more typical. Two places used
with_boxed_chain(),which doesn't have an equivalent in
anyhow, so I did my best to dosomething reasonable.
In
src/server.rswe now importstd::task::ContextasTaskContexttoavoid overshadowing the
anyhow::Contexttrait :(