Skip to content

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jun 14, 2025

Implements rust-lang/libs-team#335.

Tracking issue: #142503

Notable differences from ACP:

  • trim_trailing_sep was added to Path since it felt reasonable to ensure that the inverse operation was available.
  • Per suggestion of @kennytm, added push_trailing_sep and pop_trailing_sep to PathBuf in addition to set_trailing_sep.

This also updates some of the docs on various Path methods to use the term "trailing separator" instead of "trailing slash" for consistency.

@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 14, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the path-trailing-sep branch 2 times, most recently from 9547ab7 to c5f456e Compare June 15, 2025 17:12
@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

@rustbot author

@rustbot rustbot 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 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 15, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Jun 29, 2025

Now that I know the workaround for the ./x test library/std, this should be good. But I'll wait until the tests pass (I'm only running on Linux atm) before I mark as ready.

There are a few edge cases, namely removing the trailing separator from /.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Jun 29, 2025
@clarfonthey clarfonthey force-pushed the path-trailing-sep branch 2 times, most recently from 22c4da0 to 840b3a0 Compare August 10, 2025 05:20
@bors
Copy link
Collaborator

bors commented Aug 13, 2025

☔ The latest upstream changes (presumably #145334) made this pull request unmergeable. Please resolve the merge conflicts.

/// assert_eq!(Path::new("dir/").trim_trailing_sep().as_os_str(), OsStr::new("dir"));
/// assert_eq!(Path::new("dir").trim_trailing_sep().as_os_str(), OsStr::new("dir"));
/// assert_eq!(Path::new("/").trim_trailing_sep().as_os_str(), OsStr::new("/"));
/// assert_eq!(Path::new("//").trim_trailing_sep().as_os_str(), OsStr::new("//"));
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? I would have thought that since // is the same as /, this would trim // to / (but feel free to correct me).

Copy link
Contributor Author

@clarfonthey clarfonthey Aug 13, 2025

Choose a reason for hiding this comment

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

The idea here is that removing any number of /s would not affect the final path, and so, the path is left as-is. Essentially, any sequence of separators is treated as a single separator for the logic.

I could change it so that it returns / here but it felt better to be conservative: the logic is that trailing separators are removed if doing so would result in the same path without a trailing separator. Since there will always be trailing separators here, it doesn't make a difference to remove any. If your goal is "cleaning up" the path, then normalize and normalize_lexically exist instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also another way to word this: x.trim_trailing_sep().as_os_str() == x.as_os_str() and x.trim_trailing_sep().has_trailing_sep() are equivalent.

(Note that the as_os_str is required since the trailing separator is ignored when comparing paths.)

Copy link
Member

Choose a reason for hiding this comment

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

This makes sense to me, but I'll ask for libs-api feedback just in case.

@rustbot

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Aug 21, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@joboet joboet added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants