Rework Error type #583
Merged
Rework Error type #583
Conversation
This file contains 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
danielocfb
force-pushed
the
topic/error
branch
3 times, most recently
from
a14a62c
to
5d8c431
Compare
Our existing Error type is sub-optimal in many ways: 1) using an exhaustive enum we can't easily extend it without breaking backwards compatibility 2) the provided variants are rather subjective and not useful: what is a system error? Why is it relevant that something is a system error vs. something else? What could be more relevant to users, would be the kind of error encountered, e.g., permission denied or object not found. Similarly, what is an internal error? If an internal invariant is violated that's a bug, but that's not how most sites use this variant. 3) it is impossible to chain errors, reporting lower level errors (such as file not found) along with higher level context (e.g., file name). This change proposes to rework the Error type completely. The type is very close to io::Error, just by virtue of us doing a lot of IO and interfacing with system-level APIs (most of which effectively report io::Error objects) and so it is a natural fit. We certainly mirror several of the std::io::ErrorKind variants in our own version of ErrorKind. On top of the error itself we also add a bunch of convenience conversion functions, mostly to provide additional context. These are heavily inspired by anyhow's Error type. That crate also acted as role model for the way we format our errors. This change only introduces the error type in a new module, error2. A subsequent one will adjust all internal consumers to use it and remove the existing Error enum. Signed-off-by: Daniel Müller <[email protected]>
This change replaces all call-sites using the previous enum based Error with the new version. For the most part it is a straight-forward conversion. Signed-off-by: Daniel Müller <[email protected]>
By now we have adjusted all users of crate-level errors to use the new Error type instead of the old one. With this change we conclude the switch by removing the old type altogether. Signed-off-by: Daniel Müller <[email protected]>
danielocfb
force-pushed
the
topic/error
branch
from
5d8c431
to
fa6d505
Compare
anakryiko
approved these changes
Sep 14, 2023
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.
Our existing Error type is sub-optimal in many ways:
backwards compatibility
system error? Why is it relevant that something is a system error vs.
something else? What could be more relevant to users, would be the
kind of error encountered, e.g., permission denied or object not
found.
Similarly, what is an internal error? If an internal invariant is
violated that's a bug, but that's not how most sites use this
variant.
as file not found) along with higher level context (e.g., file name).
This change proposes to rework the Error type completely. The type is very close to io::Error, just by virtue of us doing a lot of IO and interfacing with system-level APIs (most of which effectively report io::Error objects) and so it is a natural fit. We certainly mirror several of the std::io::ErrorKind variants in our own version of ErrorKind. On top of the error itself we also add a bunch of convenience conversion functions, mostly to provide additional context. These are heavily inspired by anyhow's Error type. That crate also acted as role model for the way we format our errors.
This change only introduces the error type in a new module, error2. A subsequent one will adjust all internal consumers to use it and remove the existing Error enum.