Skip to content
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

Implement strongly-typed error handling #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SabrinaJewson
Copy link
Collaborator

There were a couple problems with the way the library previously did error handling:

  • Error chains didn’t inform much, as they didn’t give any context to the error
  • The library used the same error type for database loading and expansion, despite their errors beïng entirely disjoint
  • The error Display implementations didn’t follow convention

This new error API is designed with good error chains and strongly-typed errors in mind.

Examples

Examples as returned by Database::from_env():

Error: failed to load terminfo database
Caused by: no `$TERM` environment variable

Error: failed to load terminfo database
Caused by: no terminfo database found for `alacritty`

Error: failed to load terminfo database
Caused by:
   0: failed to load terminfo database /usr/share/terminfo/a/alacritty
   1: permission denied


Error: failed to load terminfo database
Caused by:
   0: failed to load terminfo database /usr/share/terminfo/a/alacritty
   1: failed to parse terminfo database

Examples as returned by expand:

Error: failed to expand terminfo string
Caused by: expansion string is invalid

Error: failed to expand terminfo string
Caused by: type mismatch

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::LoadError or terminfo::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 no source() 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 like root_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 did impl From<ParseError> for expand::Error. This was intentional under the theory that while the relationship with ParseError and expand::Error is trivial in the sense that “parsing” could only mean one thing in this context, the relationship between io::Error and expand::Error is 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant