-
Notifications
You must be signed in to change notification settings - Fork 144
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 Error for LoadingError #153
Conversation
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.
Looks good, thanks! Maybe some day syntect should switch to failure
or something.
Just one minor duplication cleanup request, but if you don't have time I'll merge this in a few days as is.
src/lib.rs
Outdated
WalkDir(ref error) => error.fmt(f), | ||
Io(ref error) => error.fmt(f), | ||
#[cfg(feature = "yaml-load")] | ||
ParseSyntax(_) => write!(f, "Invalid syntax file"), |
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.
These cases could be implemented as _ => write!(f, self.description())
instead of duplicating the messages.
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.
For some reason, I didn't think it would work in this direction (because Error
depends on fmt::Display
).. but that's much better, of course. Updated.
Yes, a library that removes some of the boilerplate involved in error handling would certainly be a good idea. By the way: thank you for this amazing library! |
src/lib.rs
Outdated
ParseTheme(_) => write!(f, "Invalid syntax theme"), | ||
ReadSettings(_) => write!(f, "Invalid syntax theme settings"), | ||
BadPath => write!(f, "Invalid path"), | ||
ParseSyntax(_) => write!(f, "{}", self.description()), |
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.
Still not a fan of this duplication. I'd prefer either a _
pattern or if you really want the exhaustiveness checking (I don't think it's important in this case since description is a fine default) you can use a separate item for ParseSyntax
and then ParseTheme(_) | ReadSettings(_) | BadPath =>
for the rest.
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.
Sorry.. just now saw your _ =>
prefix in the first comment. I've updated it to a catch-all pattern.
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.
Thanks!
match *self { | ||
WalkDir(ref error) => error.fmt(f), | ||
Io(ref error) => error.fmt(f), | ||
_ => write!(f, "{}", self.description()), |
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 think this should not rely on Error::description()
since it is going to be deprecated soon?
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 didn't know that.
Anyhow, it's still required for now and we can easily refactor it to remove it once the default implementation has been stable for a while, or we switch to failure
.
Implements
std::error::Error
forLoadingError
.This is a first step towards #94. Things could definitely be improved by also implementing
Error
for the other syntect-specific errors.