manuel-woelker / rust-vfs

A virtual filesystem for Rust
Apache License 2.0
384 stars 44 forks source link

Improve error ergonomics for end users #34

Closed Technohacker closed 2 years ago

Technohacker commented 2 years ago

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

Technohacker commented 2 years ago

Marked as a draft due to the test suite being broken. There's no longer a From<io::Error> impl for VfsError since it also needs the path where the error occured. Hence each call site needs to map_err to VfsError manually. Any other possible solutions?

manuel-woelker commented 2 years ago

Hi, sorry for the late reply.

It might be possible to implement From<io::Error>, leaving the path blank initially and have it be filled in not by the FS impl, but by the VFS infrastructure. This would mean that the path is the VFS path not the physical filesystem path. This would make it easier on FS implementors, and would keep the error paths the same across different FS implementations.

Technohacker commented 2 years ago

Ah that's a good idea, I'll try that out

Technohacker commented 2 years ago

Apologies for the delay, couldn't get back to this earlier :sweat_smile:

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 :)

manuel-woelker commented 2 years ago

Great, thanks for the update, I'll try to get a look at the changes tomorrow.

manuel-woelker commented 2 years ago

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?

Technohacker commented 2 years ago

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?

manuel-woelker commented 2 years ago

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.

Technohacker commented 2 years ago

Alright in that case for now, this should be good :) Pretty sure this will be a breaking change, so that's something to note :sweat_smile: