app-dirs-rs icon indicating copy to clipboard operation
app-dirs-rs copied to clipboard

Accept paths as AsRef<Path>.

Open jimmycuadra opened this issue 8 years ago • 3 comments

This patch changes the path parameters to functions from &str to AsRef<Path>. The benefit of doing this I see is:

  1. The same function will be able to accept several different types as input, notably Path and PathBuf.
  2. Manipulating paths internally is probably a safer approach than using string slices, since the Path API already provides cross-platform path behavior not inherent to strings.

This is a work in progress and not really intended to be merged as-is. Glancing quickly at the test output, I don't even think it's working yet. But if you want to make this change I can make sure it works and clean it up according to whatever style you prefer.

jimmycuadra avatar Oct 28 '16 11:10 jimmycuadra

I originally used AsRef<Path>, but I had an issue with Windows where passing in "foo/bar" would return something like "C:\backslashes\until/foo/bar". I'd be more than willing to use the more idiomatic AsRef<Path> if we can make sure forward slashes will be converted to the proper path separator.

I think mixed slash paths might work fine, but it feels gross to return malformed paths. And apologies if you've actually handled this - can't look at the code atm, away from computer.

andybarron avatar Oct 28 '16 19:10 andybarron

NB: std::fs::canonicalize might fix the issue with mixed slashes; I'll have to test it on my Windows machine.

andybarron avatar Oct 28 '16 20:10 andybarron

Native Windows API will be fine with "C:\backslashes\until/foo/bar", though I don't know if Rust will care.

fs::canonicalize on Windows will return UNC paths, which aren't suitable for further manipulation (eg if I the caller want to push() a filename to the path) since they disable normalization. So ideally you should not return normalized paths.

let foo = PathBuf::from(r"C:\foo\bar");
let mut foo = foo.canonicalize().unwrap();
foo.push(".");
println!("{:?}", foo); // r"\\?\C:\foo\bar\."
foo.canonicalize(); // Error { repr: Os { code: 123, message: "The filename, directory name, or volume label syntax is incorrect." } } since UNC paths disable normalization

Arnavion avatar Feb 05 '17 01:02 Arnavion