Tracking Issue for PathBuf::add_extension and Path::with_added_extension

[tisonkun]

Feature gate: #![feature(path_add_extension)]

This is a tracking issue for

• PathBuf::add_extension
• Path::with_added_extension

Public API

impl PathBuf {
    pub fn add_extension<S: AsRef<OsStr>>(&mut self, extension: S) -> bool { ... )
}

impl Path {
    pub fn with_added_extension<S: AsRef<OsStr>>(&self, extension: S) -> PathBuf { ... }
}

Steps / History

• ACP: impl PathBuf::add_extension and Path::with_added_extension libs-team#368
• Implementation: impl PathBuf::add_extension and Path::with_added_extension #123600
• Final comment period (FCP)
• Stabilization PR

Unresolved Questions

None yet.

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[MordechaiHadad]

This would be really awesome honestly, just encountered an issue where this would have made my life easier.

[dilawar]

Hoping this becomes available in stable soon. Such a nice QoL api.

[anatawa12]

This new API is awesome!

But I think It would be better to rename (deprecate old and add new name) existing API with_extension with with_replaced_extension and set_extension with replace_extension along with this feature.
I feel existing function with_extension and set_extension is confusing if it will replace or append extension.

Actually I mistakenly used with_extension as with_added_extension and we can see some such case in rust form like this.

[dvdsk]

But I think It would be better to rename (deprecate old and add new name) existing API with_extension with with_replaced_extension and set_extension with replace_extension along with this feature.

I agree with the rename however we can only do that with a new edition (since with_extension is stable) right? And that might take a few more years. That should not block the stabilization of the added API.

[anatawa12]

I don't think edition change is required since we don't have to actually rename the function.
We can introduce with_replaced_extension in some version, and deprecate with_extension in following or later version.

I think there are some cases that deprecating without edition change like deprecate _ms functions with Duration parameter variants.

[joshtriplett]

Starting an FCP to stabilize this:

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[tgross35]

Some late bikeshed: the name with_added_extension feels very unnatural, what about the names push_extension and join_extension? This nicely mirrors what we have for path components:

impl Path {
    pub fn join<P: AsRef<Path>>(&self, path: P) -> PathBuf;
    pub fn join_extension<S: AsRef<OsStr>>(&self, extension: S) -> PathBuf;
}

impl PathBuf {
    pub fn push<P: AsRef<Path>>(&mut self, path: P);
    pub fn push_extension<S: AsRef<OsStr>>(&mut self, extension: S) -> bool;
}

[tisonkun]

FYI the naming discussion also happened in the ACP period - rust-lang/libs-team#368 (comment)

[Amanieu]

We discussed this in the libs-api meeting. There was a slight preference towards keeping the "add"-based names since it is consistent with set_extension and with_extension.

[tbu-]

This API currently seems to have the same issue as Path::set_extension had before introducing a panic in #125070.

This API should probably panic as well if the passed extension contains a path separator.

[tbu-]

Additional, but much less important point: The add_extension API returns a boolean which is likely not checked at call site to communicate its success in adding the extension. I think it'd be a lot better if it returned a Result which would make the user think about what to do if the file extension could not be set.

This is exemplified in the with_added_extension API which promptly forgets to check the result of add_extension and currently also has no way to return that result to the user.

[T0mstone]

I think it'd be a lot better if it returned a Result which would make the user think about what to do if the file extension could not be set.

Alternatively, the function could just be annotated with #[must_use].

[tisonkun]

This API currently seems to have the same issue as Path::set_extension had before introducing a panic in #125070.

This sounds reasonable to keep the manner in sync. As you're the author of #125070, would you submit a PR to apply the same for add_extension? Otherwise, I can submit one.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.