Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PathAbs constructor cleanup #29

Closed
Screwtapello opened this issue Aug 16, 2018 · 2 comments
Closed

PathAbs constructor cleanup #29

Screwtapello opened this issue Aug 16, 2018 · 2 comments

Comments

@Screwtapello
Copy link
Collaborator

Currently, there are a bunch of different ways to construct a PathAbs:

  • PathArc::absolute() constructs a PathAbs using lexical canonicalization
  • PathAbs::new() has a docstring that implies it uses std::fs::canonicalize() but actually calls PathArc::absolute().
  • PathAbs::mock() constructs a PathAbs but doesn't actually validate its absoluteness

I think these could be cleaned up/rationalized. For example:

  • PathAbs::new() should take a path and validate that it conforms to the rules for absolute paths, returning an error if something's wrong
    • Normally new() is for the most commonly-used constructor, but I'm not sure any of them are obviously the best
    • "validate in new()" follows standard library types like std::ptr::NonNull
  • PathAbs::new_with_canonicalization() should take a path and call std::fs::canonicalize() on it
    • Not currently a supported constructor, but if it's in the standard library people will be surprised if this crate doesn't support it at all
  • PathAbs::new_with_parents_removed() should resolve .. components by removing the previous component, like PathArc::absolute() does today
  • Delete PathAbs::mock()
    • somebody who wants a path for testing can use the tempfile crate
    • if this method is really needed, we can rename it to PathAbs::new_unchecked() after the model of std::ptr::NonNull
  • Delete PathArc::absolute()

This would also provide a natural place for PathAbs::new_with_parents_resolved() to slot in, from #20.

@vitiral
Copy link
Owner

vitiral commented Aug 16, 2018

I agree with a lot of this. Do note that releasing this will require a major version bump since it is breaking backwards compatibility.

Some comments:

  • Delete PathArc::absolute(): the reason this exists is because Path::absolute exists and this overrides it. If we remove Deref then it might not be an issue anymore. I need to think more about the part which PathArc and Deref plays.
  • PathAbs::new(): the docstring should be fixed for sure. I'm not sure if throwing an error if it isn't already an absolute path is the best thing here. I need to think about it more, but I'm leaning towards just fixing the docsftring.
  • PathAbs::new_with_canonicalization(): do you think this can be named PathAbs::canonicalized? This would be similar verbiage to the sorted std function for python. I'm not sure if there is equivalents in rust.
  • PathAbs::new_with_parents_removed(): I agree this constructor should exist but I'm not sure about the name. Some ideas: PathAbs::walked, PathAbs::resolved. resolved would kinda-sorta follow the python function, although that also resolves symlinks (but has strict=False behavior which allows non-existent paths... it's weird). The idea is that it differs from PathAbs::new by "resolving" .. elements instead of removing them syntactically. Maybe calling it PathAbs::parents_resolved would be okay as well.
  • PathAbs::mock(): I'm okay naming this new_unchecked(). I think providing a way to unit test without filesystem calls is important to this library, and new_unchecked would also let us get some performance gains for internal methods.

Also I use PathArc quite a bit for it's serialization abilities, especially when communicating with webapps. I'm not sure what the right thing to do here is. Note that the webapp use case does NOT want to actually resolve any paths, so keeping everything as PathArc is better.

It's possible that serialization in general is an overloaded feature of this library and should be split into it's own library (path_ser maybe?). I'm not totally sure but it's at least worth considering.

@Screwtapello
Copy link
Collaborator Author

Do note that releasing this will require a major version bump since it is breaking backwards compatibility.

Of course! I understand this is a big change, and compatibility must be considered too.

PathAbs::new() ... I'm not sure if throwing an error if it isn't already an absolute path is the best thing here.

I wanted to rename mock() to new_unchecked(), which implies that new() does some kind of checking, so I figured "validate this is an absolute path" would be the least surprising thing to check. That said, String::new() allocates an empty string rather than taking a &[u8] and checking for UTF-8 encoding... I thought allocating an empty path would be weird, but that's exactly what PathBuf::new() does, so there's precedent.

Of course, an empty absolute path doesn't make much sense, but the absolute version of an empty path is probably std::env::current_dir() which would actually be a somewhat useful default.

PathAbs::new_with_canonicalization(): do you think this can be named PathAbs::canonicalized?

Possibly, but constructors in Rust tend to be named with_foo() or from_bar() or similar, and I was trying to come up with a name that fit that scheme.

PathAbs::new_with_parents_removed(): I agree this constructor should exist but I'm not sure about the name.

To be clear, I wanted to use this name for the current behaviour of PathAbs::new(), lexically resolving parent references. The other algorithm I worked on (resolving symlinks followed by ..) would be PathAbs::new_with_parents_resolved(). Hopefully, somebody looking at "parents removed" and "parents resolved" next to each other in the documentation would get a big hint about how those two similar methods are different, and why somebody might choose one over the other.

It's possible that serialization in general is an overloaded feature of this library and should be split into it's own library (path_ser maybe?).

I'm 100% on board with having serialisation support in path_abs. For one thing, it's a recommendation in the Rust API guidelines, and for another I've used crates where serialisation is a feature in a separate crate (specifically url and url_serde) and it was a pain.

vitiral pushed a commit that referenced this issue Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants