-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
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:
Also I use It's possible that serialization in general is an overloaded feature of this library and should be split into it's own library ( |
Of course! I understand this is a big change, and compatibility must be considered too.
I wanted to rename Of course, an empty absolute path doesn't make much sense, but the absolute version of an empty path is probably
Possibly, but constructors in Rust tend to be named
To be clear, I wanted to use this name for the current behaviour of
I'm 100% on board with having serialisation support in |
Currently, there are a bunch of different ways to construct a
PathAbs
:PathArc::absolute()
constructs aPathAbs
using lexical canonicalizationPathAbs::new()
has a docstring that implies it usesstd::fs::canonicalize()
but actually callsPathArc::absolute()
.PathAbs::mock()
constructs aPathAbs
but doesn't actually validate its absolutenessI 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 wrongnew()
is for the most commonly-used constructor, but I'm not sure any of them are obviously the bestnew()
" follows standard library types likestd::ptr::NonNull
PathAbs::new_with_canonicalization()
should take a path and callstd::fs::canonicalize()
on itPathAbs::new_with_parents_removed()
should resolve..
components by removing the previous component, likePathArc::absolute()
does todayPathAbs::mock()
PathAbs::new_unchecked()
after the model ofstd::ptr::NonNull
PathArc::absolute()
PathArc
is kind of a behind-the-scenes type (see Move PathArc to botom of docs and make it more clear that it is an implementation detail #18) and doesn't need any more attention drawn to itPathArc
into aPathAbs
can use one of the direct constructorsThis would also provide a natural place for
PathAbs::new_with_parents_resolved()
to slot in, from #20.The text was updated successfully, but these errors were encountered: