Implement strongly-typed error handling - #37
Open
SabrinaJewson wants to merge 1 commit into
Open
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
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.
There were a couple problems with the way the library previously did error handling:
Displayimplementations didn’t follow conventionThis new error API is designed with good error chains and strongly-typed errors in mind.
Examples
Examples as returned by
Database::from_env():Examples as returned by
expand:Unfortunately, we do end up with significantly many more error types in our public API. I believe this to be an acceptable cost since there are inherently quite a lot of different ways operations can fail and this is how to achieve nice-looking error source chains. Additionally, they’re all cordoned off in their different modules (for example
terminfo::database::LoadErrororterminfo::expand::StackUnderflow).Design note 1: I used quite a lot of
#[non_exhaustive]unit structs in the implementations of these errors. The number of types overall could be reduced by combining these into enums, but unit structs for the root-cause error types, i.e. the ones with nosource()that don’t end in*Error, i.e.terminfo::{expand::{TypeMismatch, StackOverflow}, database::{NoTerm, NotFound}}, have the advantage that one can downcast directly into them from functions likeroot_cause.Design note 2: I didn’t put
#[non_exhaustive]on most the enums. This was intentional — I guessed that it’s unlikely that new variants would need to be added in future. However I could be wrong about this.Design note 3: I didn’t
impl From<io::Error> for expand::Error, even though I didimpl From<ParseError> for expand::Error. This was intentional under the theory that while the relationship withParseErrorandexpand::Erroris trivial in the sense that “parsing” could only mean one thing in this context, the relationship betweenio::Errorandexpand::Erroris nontrivial in the sense it’s not immediately clear what “I/O” is beïng talked about if it is done by the expansion process. As a concrete example of a different I/O operation that could occur, one thing this library does not implement (AFAICT) is millisecond delays during expansion, but I think (from reading the manpage) they’re technically sometimes required.