andrewhickman / fs-err Goto Github PK

This crate fills an important hole in the ecosystem and that a lot of people would use it if they knew about it and its benefits.

I'd like to help draft an expanded README along with crate-level documentation and examples. I want to make sure my understanding of fs-err's role matches yours, though.

Key points would be:

• fs-err is a (mostly) drop-in replacement for std::fs, so it could be used like use fs_err as fs to get existing code working
• We should have clear comparisons of error messages between std::fs and fs-err
• The crate's scope is the same as std::fs, plus methods to expose paths where we've added them
• There might be a path for merging some of the work in this crate back into std (?)

This would cover remove_file, remove_dir, and remove_dir_all.

std::fs::File uses AsRef<Path> as the type for passing paths while fs_err uses Into<PathBuf>

This makes fs_err not be a "drop in replacement": When I pass an AsRef<Path> to fs_err::File::open, it does not compile

Hello!

I created a similar wrapper to fs-err, but I took a slightly different approach. When using a custom error type, code that uses those I/O functions has return types that don't compose well with the io trait ecosystem like io::Write.

If we use io::Error's ability to hold other arbitrary error types though, we can still return io::Error but attach custom information with no loss of error handling ability.

Here's an implementation I did recently: https://github.com/rojo-rbx/foreman/blob/4cca3ba84025ef42eb458f2a8184f93a42915c15/src/fs.rs

I was going to make a crate for this last week, but then found fs-err! Do you think this strategy would work better, and if so, can I help move fs-err to that pattern?

The docs of the fs-err wrappers currently just say "Wrapper for ". While that's true and easy to maintain, it is also a bit underwhelming for IDE users who rely on hovering over a function to see what it does.

For example, if I hover over a call to fs_err::OpenOptions::create_new() I just get "Wrapper for OpenOptions::create_new". If I instead hover over std::fs::OpenOptions::create_new(), I get a description such as "Sets the option to create a new file, failing if it already exists. [...]". While the shown docs is technically 100% true, it's less useful, and means that swapping std::fs for fs_err makes the code less convenient to browse in IDE.

Could we make a compromise between maintainability and usefulness by copying the first sentence of the upstream docs? Something like "Sets the option to create a new file, failing if it already exists. Wrapper for OpenOptions::create_new, which see for details."

Or shall it stay English-only?

Rust documentation at https://doc.rust-lang.org/std/fs/struct.File.html#method.set_modified

Similarly to read_to_string, we may want to reimplement the guts of create_dir_all inside this crate so that we can report more accurate paths than just the top-level path in the event of an error.

Looking at std::fs, for the functions I was using in my project, I was missing hard_link, soft_link, rename. Could these be added?

This should be a little easier than #17 to get right.

Currently there is no API that allows mutable access to some of those files or conversion to the inner std::fs::File. As such one has to go via the os-specific {FromInto}RawFd to gain access to the underlying file.

This is required for i.e. the tar crate.

It would be useful to keep track of a changelog of releases, and have people filing PRs (oops) append to an 'unreleased' section.

I like the rough structure of Keep a Changelog, but am not really tied any specific organization.

The project is very cool for applications that can tolerate synchronous filesystem interaction!
However, if one needs async io, fs_err can't help with this too much.
As a workaround some operations could be wrapped in tokio::task::spawn_blocking(), but it would be better to have a native wrapper for tokio::fs module.

It may be reasonable to have this with a tokio cargo feature.

Thank you for your work. My team works with the file system heavily, and we need good error messages that include the path being referenced, and your library is helpful in providing this out of the box.

One thing we've found is that while checking if a file exists or not on the path, it can be confusing to raise an error that says "This file {} does not exist" when the end user can see it on the path but Path::exists() returned false because there was an error with permissions (or something else).

Currently, I have some code like this:

    if path
        .try_exists()
        .map_err(|error| CustomError::CannotRead(path.clone(), error))?
    {
        Ok(path)
    } else {
        Err(CustomError::DoesNotExist(path))
    }

If fs-err supported these operations, then this would be simplified to just path.try_exists()?.

The challenge is how to present this as an API. Maybe something like FsErrPath::try_exists(&path)?

I would like to make "doing the right thing" for error ergonomics as easy as possible. What you've got is a great start. Would you consider adding some additional io operations to your library?

There exist several structs in std::fs which don't appear in fs_err

• DirBuilder
• Metadata
• FileType
• Permissions

Of these, the first 2 have methods returning io::Result<_> which would benefit from wrappers.
The last 2 don't have any fallible methods. If fs-err re-exported them, it would add to the "drop-in" quality of the crate. The downside is that if std ever adds fallible methods later, they'd need to be replaced with wrappers in a breaking change.

In the case of Metadata, there already are several APIs which return std::fs::Metadata. When Metadata is wrapped, these should be switched to return the wrapper (breaking change).

THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!
THANK YOU!!!

You have no idea how amazing it is to see which file was not found!

The format for errors from fs::copy are a little different because there are two paths involved. I solved this in my FS abstraction crate by having a new internal error type, CopyError.

I can PR this; I'll need it to port some projects over to use fs-err!

It would be useful to expose the read_dir function, along with the associated structs ReadDir (an iterator) and DirEntry.

I have implementations of these in my old FS abstraction (that was the source of #2) that I can tidy up and PR for this crate!

The standard library implements remove_dir_all in terms of multiple operations. We should provide an error message detailing exactly which of these failed.

This is straightforward on most platforms. The implementation can be replicated using existing APIs. Unfortunately the windows implementation uses the unstable FileTypeExt::is_symlink_dir

We can work around this by using winapi directly, or waiting for the method to stabilize

When printing out the errors generated by fs-err with Display, the message does not contain the source. This often means we know what happened, but not why an error happened.

For example, I encountered this when a call to fs_err::hard_link() failed. I have an error hierarchy that wraps the error from the hard link call. Printing the error with Display shows my custom text, then the text from fs-err, but not the text from the source/underlying std::io::Error.

Here's an example in the Playground (please pardon the unpolished nature of the code):
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a5e0844e243ec0807372b01227419b50

The Display1 shows wrapping a normal std::io::Error. We can see the output does not include the path. So this has the why, but no what.

In Display2, which is what fs-err does, we have the path, but not the underlying source io::Error included in the output. This is having the what without the why

And Display3 is what I was hoping that fs-err did: it prints the what followed by the why.

By adding the source at the end of the Display, this automatically keeps printing more and more inner source errors at the end; providing more and more information/context.

For fs-err, this can be accomplished by changing the implementation of Display from here:

To something like:

    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        use ErrorKind::*;

        let path = self.path.display();

        match self.kind {
-            OpenFile => write!(formatter, "failed to open file `{}`", path),
+            OpenFile => write!(formatter, "failed to open file `{}`: {}", path, self.source),
            // and the same thing for the subsequent arms

I can create a PR for this; just wanted to check if this would be well-received first. Please also let me know if there is more information that would be helpful.

Edit: I created a PR since it was quite quick: #53

This is a fairly common API and one I need for a couple of my projects.

It would be nice if something analogous to tokio_stream::wrappers::ReadDirStream is available for fs_err::tokio::ReadDir

A lot of functions in this crate are generic over T: AsRef. This shifts compile time to the callers of this crate, and may lead to repeated compilations of these functions.

The way to avoid is is to immediately delegate to non-generic function. See std for an example:

https://doc.rust-lang.org/stable/src/std/fs.rs.html#267-275