Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Error Handling #438

Merged
merged 9 commits into from
Aug 27, 2020
Merged

Conversation

msrd0
Copy link
Member

@msrd0 msrd0 commented May 28, 2020

This Pull Request removes all usages of the now outdated failure crate and replaces them with anyhow, the suggested successor for a generic error type.

It also aligns the API of NewMiddleware with the API of NewHandler. Since the user does not get to handle the error types but all we do is generate an error response, we don't need a concrete error type like std::io::Error but can use a generic one. This simplifies the use of non-io errors since they can now use the shorthand ? operator instead of going the considerably more elaborate io::Error::Other route.

This is a breaking API change but I expect the required changes to be minimal. Most users don't have many manual implementations of NewHandler or NewMiddleware, and for those implementations, all they have to do is change io::Result into anyhow::Result (which is re-exported by gotham).

@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #438 into master will decrease coverage by 0.27%.
The diff coverage is 66.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
- Coverage   84.44%   84.17%   -0.28%     
==========================================
  Files         110      110              
  Lines        5479     5473       -6     
==========================================
- Hits         4627     4607      -20     
- Misses        852      866      +14     
Impacted Files Coverage Δ
examples/handlers/async_handlers/src/main.rs 93.57% <0.00%> (ø)
examples/handlers/form_urlencoded/src/main.rs 80.55% <0.00%> (-2.78%) ⬇️
examples/handlers/multipart/src/main.rs 72.72% <0.00%> (-0.41%) ⬇️
examples/handlers/request_data/src/main.rs 85.18% <0.00%> (-1.86%) ⬇️
examples/handlers/stateful/src/main.rs 83.67% <ø> (ø)
gotham/src/lib.rs 82.75% <ø> (ø)
gotham/src/middleware/logger/mod.rs 0.00% <0.00%> (ø)
gotham/src/middleware/security/mod.rs 0.00% <0.00%> (ø)
gotham/src/middleware/timer/mod.rs 0.00% <0.00%> (ø)
gotham/src/pipeline/chain.rs 70.00% <0.00%> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ba0b3c...737b64e. Read the comment docs.

pksunkara
pksunkara previously approved these changes Jun 3, 2020
Copy link
Contributor

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I didn't find any issues. I will wait for while to let @nyarly or @colinbankier review this.

@msrd0
Copy link
Member Author

msrd0 commented Jun 24, 2020

@nyarly @colinbankier @pksunkara any updates on this?

@msrd0
Copy link
Member Author

msrd0 commented Jul 6, 2020

Update: I have now also refactored HandlerError to have a From<E> implementation for all error types that implement Into<anyhow::Error> + Display. This allows to use all types that implement std::error::Error plus anyhow::Error itself as the error type for handlers, with ? shorthand support being completed with #450. Also, for convenience, Result<T, E> and TryFuture<T, E> have a map_err_with_status(status) method that is basically equivalent to map_err(|err| HandlerError::from(err).with_status(status)).

CC @tanriol

@msrd0 msrd0 requested a review from pksunkara July 6, 2020 22:40
@msrd0 msrd0 mentioned this pull request Aug 26, 2020
@msrd0 msrd0 added this to the 0.5 milestone Aug 26, 2020
@sezna
Copy link
Collaborator

sezna commented Aug 26, 2020

So there are two options to enable this -- we can either loosen or remove the code coverage requirement, or you can add more test coverage. Obviously the latter is preferred, but only if it is practical.

@msrd0
Copy link
Member Author

msrd0 commented Aug 26, 2020

Afaik, code coverage is optional and merging is blocked because this pull request has no review (and also because there were merge conflicts which I just resolved). If you think some part of this PR is poorly tested, please let me know.

@sezna
Copy link
Collaborator

sezna commented Aug 26, 2020

I didn't realize the codecov check was non-blocking. I'll look at this.

@sezna sezna self-requested a review August 26, 2020 13:40
sezna
sezna previously approved these changes Aug 27, 2020
Copy link
Collaborator

@sezna sezna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@msrd0
Copy link
Member Author

msrd0 commented Aug 27, 2020

I resolved another merge conflict, should be good for merge now @sezna

@msrd0 msrd0 merged commit b2275a4 into gotham-rs:master Aug 27, 2020
@msrd0 msrd0 deleted the improve-error-handling branch August 27, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants