-
Notifications
You must be signed in to change notification settings - Fork 125
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
@nyarly @colinbankier @pksunkara any updates on this? |
Update: I have now also refactored CC @tanriol |
08292b5
to
16564bc
Compare
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. |
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. |
I didn't realize the codecov check was non-blocking. I'll look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I resolved another merge conflict, should be good for merge now @sezna |
This Pull Request removes all usages of the now outdated
failure
crate and replaces them withanyhow
, the suggested successor for a generic error type.It also aligns the API of
NewMiddleware
with the API ofNewHandler
. 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 likestd::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 elaborateio::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
orNewMiddleware
, and for those implementations, all they have to do is changeio::Result
intoanyhow::Result
(which is re-exported by gotham).