Normalization does not seem to work on windows?

[matklad]

        eprintln!("\n{}", path);
        eprintln!("{:?}", RelativePath::new(path));
        eprintln!("{:?}", RelativePath::new(path).normalize());

prints

lib.rs\..\foo.rs
"lib.rs\\..\\foo.rs"
"lib.rs\\..\\foo.rs"

I would expect to see foo.rs as the third entry, and I indeed get this behavior on Linux.

[udoprog]

Hey!

From the README:

RelativePath only uses / as a separator. Anything else will be considered part of distinct components.

This constraint is in place to permit for a the same relative path representation across platforms.

The way you'd deal with platform-specific relative paths is to convert them to a using RelatiavePathBuf::from_path which does this on a component-by-components basis. This also verifies that the path is not absolute.

i.e.:

eprintln!("\n{}", path);
eprintln!("{:?}", RelativePath::from_path(Path::new(path)).expect("bad path"));
eprintln!("{:?}", RelativePath::from_path(Path::new(path)).expect("bad path").normalize());

[matklad]

Ah, I see, thanks!

I've actually suspected this, but I've failed to locate from_path method in the docs (not that I've tried to search to hard), and wrongly concluded that RelativePath::new is what I am looking for.

Perhaps this means that new method is a footgun and should be deprecated? Specifically, I think its very reasonable to assume that creating a RelativePath from &str is the same as creating std::Path and then converting it to a RelativePath.

[udoprog]

Hm. There are a couple of points is like to raise that speaks against new performing a conversion.

So due to the current semantics, conversion from a Path is a fallible operation. It's not legal to cover a platform-specific absolute path into a RelativePath. This would mean that new is a fallible operation.

The existing new performs no conversion. It's a wrapper around a str, similarly to how Path is a wrapper around an OsStr.

The API mimics how the Path API works. It did confuse a seasoned Rust dev, which speaks to it not being as intuitive as one might want. But I also do think the opening to the documentation should help to build an intuition around this. Could you look at this @matklad?

So I'm thinking this might be a documentation problem or an API problem. What do you think?

[matklad]

So I'm thinking this might be a documentation problem or an API problem. What do you think?

Documentation certainly won't hurt, yeah. However, API-wise I personally would a prefer a fallible fn new(&str) -> Result<&RelativePath> and, probably, fn new_unchecked(&str) -> &RelativePath with the current semantics.

I think that the failable new is also required to implement this logic: "I have a &str, which is a system-path, I want a relative Path out of it, but I don't want to allocate if I don't have to".

It could work like this:

let s: &str = ...

let path_buf;
let path: &RelativePath = RelativePath::new(s).unwrap_or_else(|| {
    path_buf = RelativePathBuf::from_path(s); // allocates
    path_buf.as_path()
});

[udoprog]

I think I understand what you want to accomplish: A convenient and cheap way of converting a system path into a relative path.

In the example you brought up there is one weirdness: the internal representation of RelativePath relies on having forward slashes as the path separator, that would make the expression platform-specific for when the conversion is cheap vs. when it's not.

Given that from_path currently only exists on RelativePathBuf, I think it would be reasonable to add RelativePath::from_path, which attempts to perform a cheap conversion with checks and without allocations the way you want it.

It would have to do something like this:

impl RelativePath {
  fn from_path<'a, P: AsRef<Path> + ?Sized>(path: &'a P) -> Result<&'a RelativePath, FromPathError> {
    // 1. check that path is an absolute path

    // 2. check that path uses forward-slashes for its component separator.

    // 3. use Path::as_os_str and OsStr::to_str on the result to verify that the path is a valid string

    // 4. wrap and return this with Ok(RelativePath::new(s))
  }
}

With this, your example would look like this:

let s: &str = ...
let mut path_buf = ...

let path: &RelativePath = RelativePath::from_path(s).or_else(|| {
    path_buf = RelativePathBuf::from_path(s)?; // allocates, note that this is also fallible.
    Ok(path_buf.as_relative_path())
})?;

I hope it's a start without having to break the API. I'll have to do a bit more thinking before I commit to changing new.

[matklad]

Sounds fine to me as long as there's an example like RelPath::new("foo\\bar").component().count() == 1 in the docs!