fs: add new error type #792
Conversation
phip1611
force-pushed
the
fs-err
branch
4 times, most recently
from
e5203be to
d6c05e6
Compare
|
@nicholasbishop this is now ready for review - what do you think? Is this helpful or overengineered? |
uefi/src/fs/file_system/error.rs
Outdated
|
|
||
| /// Logical errors when working with files. | ||
| #[derive(Debug, Clone, Display, PartialEq, Eq)] | ||
| pub enum LogicError { |
There was a problem hiding this comment.
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.
I didn't know about std::io::ErrorKind, interesting!
uefi/src/fs/file_system/error.rs
Outdated
| /// UEFI-error with context when working with the underlying UEFI file protocol. | ||
| #[derive(Debug, Clone, Display, PartialEq, Eq)] | ||
| #[display(fmt = "FileSystemIOError({},{})", context, path)] | ||
| pub struct FileSystemIOError { |
There was a problem hiding this comment.
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.
Thanks! I've updated the PR
|
Just making sure you saw -- CI failed. (Also there's an |
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