-
-
Notifications
You must be signed in to change notification settings - Fork 178
fs: add new error type #792
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
Conversation
e5203be to
d6c05e6
Compare
@nicholasbishop this is now ready for review - what do you think? Is this helpful or overengineered?
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.
Splitting up the error type looks good to me. Left a couple minor suggestions
uefi/src/fs/file_system/error.rs
Outdated
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 the LogicError variants should be merged into the IoError variants to simplify a bit (and that matches how std::io::ErrorKind works)
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 about std::io::ErrorKind, interesting!
uefi/src/fs/file_system/error.rs
Outdated
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.
Naming nit: since this is already in the fs module, I think we can drop the FileSystem prefix from this and FileSystemIOErrorContext.
Also, per https://rust-lang.github.io/api-guidelines/naming.html, acronyms a treated as a word for capitalization, so IoError instead of IOError.
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! I've updated the PR
Just making sure you saw -- CI failed. (Also there's an a commit that was probably meant to be squashed.)
Uh oh!
There was an error while loading. Please reload this page.
In #472 I added an initial version of a file-system error type. I did not really like it. Therefore, this MR adds a new type which adds much more context to errors. I think, the new abstraction is helpful - yet kind of feature-creep 😁
What do you think?
Hints for Review
Changelog update is not needed I think, as none of the fs-related code is published yet.
Checklist