Closed tisonkun closed 5 days ago
+1 but your "dry-run" code expected copied
being a PathBuf
, yet your proposed add_extension()
returns a bool. I suppose the simplified code should be:
let mut copied = doc.filepath.clone();
copied.add_extension("formatted"); // should actually handle a `false` result
doc.save(Some(&copied));
Alternatively (additionally?) maybe you actually want a &Path -> PathBuf
method
impl Path {
fn with_extra_extension(&self, extension: impl AsRef<OsStr>) -> PathBuf { ... }
}
let copied = doc.filepath.with_extra_extension("formatted");
doc.save(Some(&copied));
@kennytm Thanks for your input. Correct that my sample code has a bug.
I agree that we can additionally add a Path:: with_extra_extension
method, since if the case is it can modify the PathBuf
in place, the user can avoid construct a new instance.
Let me update in the PR and issue description.
@joboet is there a specific timeline that lib-team would pick up this issue? Or how can I add this issue in the schedule?
Just be patient. There's quite a backlog but they'll get to it eventually.
We discussed this in today's libs-api meeting. We agreed that we do want to add these.
One naming tweak: we'd like to name the Path
method with_added_extension
, for consistency with PathBuf::add_extension
.
Thanks for your updates! Let me create a tracking issue in the main repo and update the patch correspondingly.
Somehow I found this code snippet:
pub trait PathBufExt {
/// Append an extension to the path, even if it already has one.
fn with_extra_extension<S: AsRef<OsStr>>(&self, extension: S) -> PathBuf;
}
impl PathBufExt for PathBuf {
fn with_extra_extension<S: AsRef<OsStr>>(&self, extension: S) -> PathBuf {
if extension.as_ref().is_empty() {
self.clone()
} else {
let mut fname = self.file_name().unwrap().to_os_string();
if !extension.as_ref().to_str().unwrap().starts_with('.') {
fname.push(".");
}
fname.push(extension);
self.with_file_name(fname)
}
}
}
So I'll keep the name with_extra_extension
for now and open to comments.
Updated at https://github.com/rust-lang/rust/pull/123600. PTAL.
Source of that code snippet: https://github.com/rust-lang/rust/blob/66b4f0021bfb11a8c20d084c99a40f4a78ce1d38/src/tools/compiletest/src/util.rs#L36-L54
A name chosen by the internal "compiletest" tool should not be used as justification to "keep the name with_extra_extension
".
OK. Then I can update the method name. It just to reduce the changeset in the first place.
Proposal
Problem statement
Sometimes, the program can generate files with its own suffix (extension) to identify specific purpose files.
For example, in my tools there is a dry-run mode for formatting files (link):
If we have an
add_extension
method here to append extra extension, the code can be simplified as:This situation can be applied for
.bak
or other cases.PathBuf::add_extension(&mut self, extension: impl AsRef<OsStr>)
is an additional method to modify the PathBuf in place without construct a brand-new instance.Motivating examples or use cases
Included above.
Solution sketch
It's more expressive with a patch; see https://github.com/rust-lang/rust/pull/123600:
Alternatives
Not applicable. This is a trivial case somewhat.
Links and related work
https://github.com/rust-lang/rust/pull/123600:
What happens now?
This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
Possible responses
The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):
Second, if there's a concrete solution: