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 file_prefix method to std::path #85166

Merged
merged 7 commits into from
Aug 22, 2021
Merged

Conversation

mbhall88
Copy link
Contributor

This is an initial implementation of std::path::Path::file_prefix. It is effectively a "left" variant of the existing file_stem method. An illustration of the difference is

use std::path::Path;

let path = Path::new("foo.tar.gz");
assert_eq!(path.file_stem(), Some("foo.tar"));
assert_eq!(path.file_prefix(), Some("foo"));

In my own development, I generally find I almost always want the prefix, rather than the stem, so I thought it might be best to suggest it's addition to libstd.

Of course, as this is my first contribution, I expect there is probably more work that needs to be done. Additionally, if the libstd team feel this isn't appropriate then so be it.

There has been some discussion about this on Zulip and a user there suggested I open a PR to see whether someone in the libstd team thinks it is worth pursuing.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I am on board with adding a method like this. Please update the implementation and open a new tracking issue.

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2021
@mbhall88 mbhall88 requested a review from dtolnay June 21, 2021 03:54
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbhall88 mbhall88 requested a review from dtolnay June 24, 2021 04:27
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@mbhall88 - can you please mark dtolnay's change requests as addressed?

@mbhall88
Copy link
Contributor Author

Ping from triage:
@mbhall88 - can you please mark dtolnay's change requests as addressed?

@JohnCSimon I have already marked all of the comments as resolved...? Is there some other way of marking the changes as resolved?

@camelid camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 29, 2021
pub fn file_prefix(&self) -> Option<&OsStr> {
self.file_name()
.map(split_file_at_dot)
.and_then(|(before, after)| if before.is_empty() { after } else { Some(before) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first branch on this line (when before is empty) has no test coverage. What is this case for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I had it in there for the case where the filename was a single . or empty, but this is inherently handled by file_name already, so we never actually try and split on . for these cases. I've removed the branch and all tests pass. I also added another test case.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@dtolnay
Copy link
Member

dtolnay commented Aug 21, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 21, 2021

📌 Commit 51cf318 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2021
@bors
Copy link
Collaborator

bors commented Aug 22, 2021

⌛ Testing commit 51cf318 with merge 2ad56d5...

@bors
Copy link
Collaborator

bors commented Aug 22, 2021

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 2ad56d5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2021
@bors bors merged commit 2ad56d5 into rust-lang:master Aug 22, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 22, 2021
@Ziris85
Copy link

Ziris85 commented Feb 7, 2022

Hello, I just wanted to inquire about this specific function. According to the above, it was merged and should (if I'm understanding this correctly) be available as of rust v1.56.0. However, that doesn't appear to be the case, since as of rust 1.58.1, cargo will not compile code with that function, claiming it's not found:

error[E0599]: no method named `file_prefix` found for struct `PathBuf` in the current scope

>rustc -V
rustc 1.58.1 (db9d1b20b 2022-01-20)

>cargo -V
cargo 1.58.0 (f01b232bc 2022-01-19)

Just wondering if perhaps I'm simply misunderstanding when this function will be available? Thank you for your time.

@CryZe
Copy link
Contributor

CryZe commented Feb 7, 2022

It's not stable yet: #86319

@Ziris85
Copy link

Ziris85 commented Feb 7, 2022

Thanks for the quick response! The original author seems unsure what he's supposed to do next with regards to making it stable:

I don't know how these tracking issues work. The PR for this feature was merged a while ago, but I have no idea if I'm supposed to do something with this issue? @dtolnay as you were the reviewer/merger of that PR are you able to answer this?

He may need some assistance/guidance in that regard 🙂

github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
add file_prefix method to std::path

This is an initial implementation of `std::path::Path::file_prefix`. It is effectively a "left" variant of the existing [`file_stem`](https://doc.rust-lang.org/std/path/struct.Path.html#method.file_stem) method. An illustration of the difference is

```rust
use std::path::Path;

let path = Path::new("foo.tar.gz");
assert_eq!(path.file_stem(), Some("foo.tar"));
assert_eq!(path.file_prefix(), Some("foo"));
```

In my own development, I generally find I almost always want the prefix, rather than the stem, so I thought it might be best to suggest it's addition to libstd.

Of course, as this is my first contribution, I expect there is probably more work that needs to be done. Additionally, if the libstd team feel this isn't appropriate then so be it.

There has been some [discussion about this on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/file_lstem/near/238076313) and a user there suggested I open a PR to see whether someone in the libstd team thinks it is worth pursuing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

10 participants