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.

Sign up for GitHub

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

std docs: edit PathBuf::set_file_name example #109540

Merged
merged 1 commit into from
May 2, 2023
Merged

std docs: edit PathBuf::set_file_name example #109540

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions library/std/src/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1395,11 +1395,16 @@ impl PathBuf {
///
/// let mut buf = PathBuf::from("/");
/// assert!(buf.file_name() == None);
/// buf.set_file_name("bar");
/// assert!(buf == PathBuf::from("/bar"));
///
/// buf.set_file_name("foo.txt");
/// assert!(buf == PathBuf::from("/foo.txt"));
/// assert!(buf.file_name().is_some());
/// buf.set_file_name("baz.txt");
/// assert!(buf == PathBuf::from("/baz.txt"));
///
/// buf.set_file_name("bar.txt");
/// assert!(buf == PathBuf::from("/bar.txt"));
///
/// buf.set_file_name("baz");
/// assert!(buf == PathBuf::from("/baz"));
Comment on lines -1398 to +1407
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be related to with_file_name and should be dropped.

Copy link
Contributor Author

@marcospb19 marcospb19 Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I changed the PR title, description, and commit message to better indicate that this is about PathBuf::set_file_name than with_file_name (but also changes with_file_name because that's a shorthand for the other method).

I can split it on different PRs but I don't think that's necessary.

Copy link
Member

Choose a reason for hiding this comment

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

To make explicit that set_file_name might replace or remove the extension, not just the file stem.

The current doc doesn't mention stem and extension at all, and it also tells that it could overwrite everything by using bar and baz.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current doc doesn't mention stem and extension at all

Exactly! That's the point of the PR, make it explicit.

Would you prefer if this is explained in the method description instead of reflected in the examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and it also tells that it could overwrite everything by using bar and baz.txt

I disagree, this is not stated as clearly as you're describing.

I'm trying to make it explicit because I was bitten by it recently.

Copy link
Member

Choose a reason for hiding this comment

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

This is ultimately a question of "What is the filename?", and to explain it from scratch would be over-explaining it. We indeed have an explanation for stem but file name and extension are common things (I believe) and it seems less important here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this is very subjective.

What you might see as "over-explaining" some simple/common concepts, I'm seeing as "filling the gaps" in the documentation for something that is non-trivial.

It's reasonable for people to assume "file name" is actually the "file stem", and that .set_file_name() doesn't overwrite the extension, so I'm being explicit here.

/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn set_file_name<S: AsRef<OsStr>>(&mut self, file_name: S) {
Expand Down Expand Up @@ -2562,7 +2567,8 @@ impl Path {
/// ```
/// use std::path::{Path, PathBuf};
///
/// let path = Path::new("/tmp/foo.txt");
/// let path = Path::new("/tmp/foo.png");
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to change the extension at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi John, the point of changing the extension here is to explicitly show that the subsequent call is overwriting the extension.

Before: foo.txt -> bar.txt.
After : foo.png -> bar.txt.

It's now clear that txt was overwritten.

Copy link
Member

@JohnTitor JohnTitor Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that the extension is unrelated to the "file name". Methods for it don't care about the extension at all, and the current doc should tell it already by using foo.txt and var. And overwriting is explained by bar.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And overwriting is explained by bar.txt.

I agree, people should/could be able to understand overwriting by the bar.txt part.

Although I can't find a real reason to revert this 3-letter change.

/// assert_eq!(path.with_file_name("bar"), PathBuf::from("/tmp/bar"));
/// assert_eq!(path.with_file_name("bar.txt"), PathBuf::from("/tmp/bar.txt"));
///
/// let path = Path::new("/tmp");
Expand Down