-
Notifications
You must be signed in to change notification settings - Fork 49
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
Error handling overhaul #49
Conversation
This yanks out all the String types in Results and replaces them with fine-grained enums. Closes Stebalien#45
/// Indicates that the terminal does not support the requested color. | ||
/// | ||
/// This is like `AttributeNotSupported`, but more specific. | ||
ColorNotSupported, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a non-exhaustive variant?
#[doc(hidden)]
__Nonexhaustive,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't ColorNotSupported
be ColorOutOfRange
(or something like that? ColorNotSupported
makes it sound like color isn't supported in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ColorOutOfRange
seems a bit weird to me as a name, since I usually think of ranges as approximating some continuous monotonic spectrum, which these colors don't. But, I can't think of a better name and agree that this is a bad name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. How about UnsupportedColor
?
On 12-13-15, cmr wrote:
- TerminfoParsing(terminfo::Error),
- /// Indicates an error expanding a parameterized string from the terminfo database
- ParameterizedExpansion(terminfo::parm::Error),
- /// Indicates that the terminal does not support the requested attribute
- AttributeNotSupported,
- /// Indicates that the
TERM
environment variable was unset, and thus we were unable to detect- /// which terminal we should be using.
- TermUnset,
- /// Indicates that we were unable to find a terminfo entry for the requested terminal.
- TerminfoEntryNotFound,
- /// Indicates that the cursor could not be moved to the requested position.
- CursorDestinationInvalid,
- /// Indicates that the terminal does not support the requested color.
- ///
- /// This is like
AttributeNotSupported
, but more specific.- ColorNotSupported,
ColorOutOfRange
seems a bit weird to me as a name, since I usually think of ranges as approximating some continuous monotonic spectrum, which these colors don't. But, I can't think of a better name and agree that this is a bad name.
Reply to this email directly or view it on GitHub:
https://github.com/Stebalien/term/pull/49/files#r47454390
Steven Allen
(310) 433-5865
((Do Not Email [email protected]))
Nice! 👏 (although the tests still need to be fixed). |
ping |
/// Indicates an error expanding a parameterized string from the terminfo database | ||
ParameterizedExpansion(terminfo::parm::Error), | ||
/// Indicates that the terminal does not support the requested attribute | ||
AttributeNotSupported, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to a more generic NotSupported
(or OperationNotSupported
)? #50 needs a way to say that querying the terminal size isn't supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with NotSupported
.
Sorry for the long delays with this, I just finished up with final exams. |
@Stebalien updated. |
No problem; I figured that might be the case. FYI, the windows terminal still needs some of it's Also, unrelated but would you like push access? This is really your crate. |
I'm happy going through pull requests (it is the GitHub Way ™️ afterall). |
26bb457
to
3ae6bb0
Compare
Thanks! |
This yanks out all the String types in Results and replaces them
with fine-grained enums.
Closes #45