Improve error ergonomics for end users #34
Conversation
|
Marked as a draft due to the test suite being broken. There's no longer a |
|
Hi, sorry for the late reply. It might be possible to implement |
|
Ah that's a good idea, I'll try that out |
Allows end users of VFS to match against ErrorKind directly without having to drill down through the WithContext variant Also offers a normalization for I/O errors to ensure the corresponding VFS Error kind is used rather than a generic I/O Error
Technohacker
force-pushed
the
error-ergonomics
branch
from
864952e
to
68076bb
Compare
|
Apologies for the delay, couldn't get back to this earlier 😅 Had to tweak my error message output to match the existing format somewhat, and some of the test cases to match the details (thanks for the error message test suite!), so that'd probably require some review VfsError now keeps a placeholder path and context if none was provided explicitly to make error handling from the implementations easier, but this means the core VFS abstraction has to manually ensure a path is filled. Maybe if we have two error types (one for the concrete impl, another for VFS itself) this could be checked at compile-time Would love to get further review on this :) |
Technohacker
force-pushed
the
error-ergonomics
branch
from
9832207
to
3f56183
Compare
Technohacker
force-pushed
the
error-ergonomics
branch
from
3f56183
to
d003bed
Compare
|
Great, thanks for the update, I'll try to get a look at the changes tomorrow. |
| let kind = match kind { | ||
| VfsErrorKind::IoError(io) => match io.kind() { | ||
| io::ErrorKind::NotFound => VfsErrorKind::FileNotFound, | ||
| // TODO: If MSRV changes to 1.53, enable this. Alternatively, |
There was a problem hiding this comment.
#[cfg(accessible(::path::to::thing))] might be an option here in the future
There was a problem hiding this comment.
Oh that's neat! However I don't think those would be usable for 1.40 since these would be newer additions to the language right?
| IoError(std::io::Error), | ||
| pub struct VfsError { | ||
| /// The path this error was encountered in | ||
| path: String, |
There was a problem hiding this comment.
I'm wondering whether it might make sense to have this actually be a Vec<String>.
This would handle multiple paths better (e.g. for copy operations).
What are your thoughts on that?
There was a problem hiding this comment.
Ideally we'd be able to pinpoint which part of the copy operation failed, whether it was reading the source or writing to the destination. I wonder if we have enough information for this?
|
Hi again, I just reviewed the PR and it looks good to me! 👍 Anything else you want to add or change, or can we go ahead and merge? |
|
Thanks! Just a few things off the top of my head :) Splitting the error type into a VFS abstraction error and an FS implementation error could be useful if we want to avoid any chance of forgetting to fill the path in VFS, though that could require a little boilerplate With the current design, is VFS in charge of filling the cause for the error or is it upto the implementations? |
|
Let's start with only a single error type for now, and address the issue of filling in the path if it turns out to be an issue. Regarding filling the cause, I'd say both. If an implementation has additional information regarding a cause, it can add that and if the VFS wants to wrap the error as cause it can do so as well. |
|
Alright in that case for now, this should be good :) |
Allows end users of VFS to match against ErrorKind directly without having
to drill down through the WithContext variant
Also offers a normalization for I/O errors to ensure the corresponding
VFS Error kind is used rather than a generic I/O Error
Fixes #33