Skip to content
This repository was archived by the owner on Apr 5, 2021. It is now read-only.

Conversation

@jimmycuadra
Copy link

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.

@andybarron
Copy link
Owner

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
Copy link
Owner

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

@Arnavion
Copy link

Arnavion commented Feb 5, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants