RUST-1406 Add type-specific errors to standard error type #555
Conversation
isabelatkinson
force-pushed
the
type-errors
branch
from
d23582a to
ec54947
Compare
|
going to change a few small things - will re-request review when this is ready again! |
| #[non_exhaustive] | ||
| pub enum ErrorKind { | ||
| /// An error related to the [`Binary`](crate::Binary) type occurred. |
There was a problem hiding this comment.
This is exposing how arbitrary it is which of our errors carry a string and which carry a subkind enum; that's got a bit of a code smell to it but I can't decide if it's actually a problem worth addressing. Do you have thoughts here?
There was a problem hiding this comment.
Yeah, I don't love this design; I agree that it's arbitrary and inconsistent. My concern, however, is that users who do have some kind of programmatic use for the values in these errors will be less inclined to migrate to the new version, so my instinct here is to err on the side of portability for lower-priority changes. Certainly open to other designs here if you had something else in mind though!
There was a problem hiding this comment.
Sketch of a proposal:
- Hoist
messageto an optional field ofError, with corresponding code in theDisplayimpl to append it if it's present. - All variants of
ErrorKindare required to be#[non_exhaustive]struct variants, even if they're empty (which a lot now will be withmessagestripped out) - Any sub-
Kindtypes follow the same pattern (sub-Kindsalso don't need to havemessagesince they're ultimately attached to anError!)
This makes the error space a lot more uniform, gives us a lot of flexibility for future error evolution, and doesn't introduce substantial migration burden.
There was a problem hiding this comment.
I like this a lot more - updated with your suggestions!
| Io(std::io::Error), | ||
| #[error("An IO error occurred")] | ||
| #[non_exhaustive] | ||
| Io {}, |
There was a problem hiding this comment.
switched this over to a stringified version of the error - std::io::Error is not Clone, and although I'm not worried about the stability of a standard library error, keeping external types out of our API allows for more flexibility in situations like RUST-1798
There was a problem hiding this comment.
We might want to carry the std::io::ErrorKind but that can be added later if it's asked for :)
| #[non_exhaustive] | ||
| pub enum ErrorKind { | ||
| /// An error related to the [`Binary`](crate::Binary) type occurred. |
There was a problem hiding this comment.
I like this a lot more - updated with your suggestions!
| Io(std::io::Error), | ||
| #[error("An IO error occurred")] | ||
| #[non_exhaustive] | ||
| Io {}, |
There was a problem hiding this comment.
We might want to carry the std::io::ErrorKind but that can be added later if it's asked for :)
This maintains the existing structure of error types that have information that could be used programatically, e.g.
oid::Error. I'm not sure how much users are actually using those values outside of printing them, but I'd rather not change that API too much unnecessarily. Type-specific errors that had a string message for each variant are collapsed into a singlecrate::error::Errorvariant for their type.