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

Add std::path::Path::is_empty. #31877

Closed
wants to merge 1 commit into from

Conversation

frewsxcv
Copy link
Member

Original issue requesting this feature:
#30259

Originally implemented in #30623
but that pull request when stale.

It was rebased in #31231, but the
Path changes got lost as the focus shifted towards OsString and
OsStr.

An RFC (rust-lang/rfcs#1497) was briefly
opened, since I didn't know if this functionality needed an RFC, but
@alexcrichton clarified in the RFC issue I linked that this is not the
case.

Original issue requesting this feature:
rust-lang#30259

Originally implemented in rust-lang#30623
but that pull request when stale.

It was rebased in rust-lang#31231, but the
`Path` changes got lost as the focus shifted towards `OsString` and
`OsStr`.

An RFC (rust-lang/rfcs#1497) was briefly
opened, since I didn't know if this functionality needed an RFC, but
@alexcrichton clarified in the RFC issue I linked that this is not the
case.
@alexcrichton
Copy link
Member

Thanks @frewsxcv! Most operations on Path operate over the components iterator, so I wonder if this also wants to do the same? I'm not sure if more than one path normalizes to "".

I'm not entirely sure what the use case is for this function, but seems fine to add for symmetry?

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 25, 2016
@alexcrichton
Copy link
Member

r? @alexcrichton

cc @aturon

@frewsxcv
Copy link
Member Author

If this does get approved, it might also make sense to add std::path::Path::len. I've personally gone back and forth on whether it makes sense to add these; I'm pretty neutral at the moment. Now that #31608 landed, getting the length and emptiness is pretty easy:

let _ = some_path.as_os_str().len();
let _ = some_path.as_os_str().is_empty();

@alexcrichton
Copy link
Member

We discusssed this in the libs team meeting yesterday, and the conclusion was that we probably don't want len and/or is_empty on Path for now. Querying a Path should exclusively go through the components iterator as that's basically how a path is always interpreted. Querying the backing storage can always be done via as_os_str (as pointed out), and that's likely good enough for now.

Put another way, testing whether a path is empty by looking at the length of the underlying bytes is probably "just the wrong question to ask", and instead the components iterator should be queried to see if it's empty.

Thanks for the PR though @frewsxcv!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants