modernize error handling #88
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rdelfin you might be interested in taking a look at this. |
|
Oooh this is nice! I'll be looking forwards to the merge. Using |
brayniac
force-pushed
the
error-handling
branch
2 times, most recently
from
2dfe265 to
35bf144
Compare
Problem Currently, we do error handling with the `failure` crate which requires that users are aware of `failure` and use it in their library or application. Solution Switch error handling to use the `thiserror` crate which allows us to derive `std::error::Error` for our error type. Result This library's public API now uses `std::error::Error` in the `Result`s. This allows better composition within higher level libraries and applications.
There was a problem hiding this comment.
That blog post is great! Using thiserror my makes a lot of sense. It not showing up in the API is great. Thanks for linking it.
I can't really makes useful comments on the actual errors as I've never done any bpf things. As for naming, being consistent would be great but I don't know which style you should go with.
|
sorry for the slow reply -- I haven't written any Rust for like a year and so I'm unlikely to be that helpful :). this seems reasonable to me! |
jvns
approved these changes
Jun 24, 2020
|
Fixing some of the error names for consistency, I will merge following successful CI run. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Currently, we do error handling with the
failurecrate whichrequires that users are aware of
failureand use it in theirlibrary or application.
Solution
Switch error handling to use the
thiserrorcrate which allows usto derive
std::error::Errorfor our error type.Result
This library's public API now uses
std::error::Errorin theResults. This allows better composition within higher levellibraries and applications.
This will be a breaking change to the existing API.
Notes to reviewers
Some of the
Errorvariants might need better naming / error messages, we should try to get naming correct on this pass to avoid additional breaking changes.Particular areas to focus on:
This PR would replace #81 - my understanding is that
anyhowwould be appropriate for an application, but we should usethiserrorwithin libraries.Came across this post, which inspired me to convert to
thiserror:https://nick.groenen.me/posts/rust-error-handling/