manuel-woelker / rust-vfs

A virtual filesystem for Rust
Apache License 2.0
374 stars 43 forks source link

Use camino paths or support non-UTF-8 paths? #22

Open sunshowers opened 3 years ago

sunshowers commented 3 years ago

Hi there -- thanks for writing this crate! I'd like to use it for some of my own crates that interact with the file system.

I'm wondering if you think using camino for paths would be interesting -- since you already restrict yourself to paths that can be encoded as UTF-8.

Alternatively, you could also go in the direction of supporting non-UTF-8 paths by using Path, OsString etc.

(I also noticed some path handling that might be funky on Windows, such as potentially not handling backslash path separators properly -- either camino or std::path::Path will help with that since they internally handle those separators.)

sunshowers commented 3 years ago

(I'm happy to do this, and also to fix up Windows support.)

sunshowers commented 3 years ago

Having looked at the impl a bit, I think I'm going to use camino to keep things simple. There's definitely some use cases that may benefit from non-UTF-8 path support, but they may wish to have a separate impl for this.

manuel-woelker commented 3 years ago

Hi, thanks for your comments and suggestions. The primary use case for this crate is abstracting over application files or assets. This means that the paths are under the control of the application and should all be UTF-8. The API currently only supports UTF-8 paths. I am not sure what benefits camino would offer. As far as I can tell, camino "frontloads" non-Unicode path errors, which is something that rust-vfs should already do out of the box.

sunshowers commented 3 years ago

Thanks! The main thing camino would provide is clearer handling around paths, and proper Windows path handling.

For example:

Let me know what you think!

manuel-woelker commented 3 years ago

Thanks for the examples!

I feel that something like root.join("foo"). is more ergonomic than root.join(camino::Utf8Path::new("foo")). I also think that if anyone wants to use camino they could either do something like root.join(camino::Utf8Path::new("foo").as_ref()) or provide an extension trait that takes a &Utf8Path

Having the VfsPath methods take impl AsRef<str> instead of &str might be the another solution that could work. I have to admit I am a bit hesitant to risk longer compile times due to method monomorphization caused by impl Parameters. I might to be falling into the "premature optimization" trap, though...

Regarding using camino internally for path handling sounds like an idea, although again I am kind of reluctant to add a dependency for an rfind. A better solution might be to refactor that logic out into an internal function or similar.

Windows path handling should work fine with the current setup. I am not currently seeing any issues on my Windows development machine. Is there anything in particular that stood out?

Thanks again for your feedback - much appreciated!

sunshowers commented 3 years ago

I feel that something like root.join("foo"). is more ergonomic than root.join(camino::Utf8Path::new("foo")). I also think that if anyone wants to use camino they could either do something like root.join(camino::Utf8Path::new("foo").as_ref()) or provide an extension trait that takes a &Utf8Path

Yeah I think impl AsRef<Utf8Path> would work best for that use case -- that's what camino's join() takes as well. That works with strings as well as paths, so it's super transparent.

Having the VfsPath methods take impl AsRef instead of &str might be the another solution that could work. I have to admit I am a bit hesitant to risk longer compile times due to method monomorphization caused by impl Parameters. I might to be falling into the "premature optimization" trap, though...

I haven't noticed a real difference tbh, and I tend to care about compile times.

Regarding using camino internally for path handling sounds like an idea, although again I am kind of reluctant to add a dependency for an rfind. A better solution might be to refactor that logic out into an internal function or similar.

Totally understood. I'm pretty biased here since I'm one of the maintainers of camino, but people have generally really appreciated the library and it already has >800k downloads and >30 dependents on crates.io in just a few months.

Windows path handling should work fine with the current setup. I am not currently seeing any issues on my Windows development machine. Is there anything in particular that stood out?

Most of the internal path processing code uses forward slashes -- in some cases people on Windows might pass in backslash-separated paths (I have a use case which does that, in fact).

Thanks again for engaging :)