chore: file.rs - Remove add_dummy_extension() helper

[polarathene]

Superseded by #682

---

The helper method is redundant (introduced as a bug fix in Mar 2022), there is a much simpler approach to append the extension.

as_mut_os_string() has been available since Rust 1.70.0, which fits this crates MSRV that is already leveraging OnceLock (since Oct 2024 / 0.14.1 release).

[polarathene]

Lint failure is about the commit title length exceeding 72 chars. I can correct that later, although I rather the project just adopt squash merge commits 😅

[epage]

. I can correct that later, although I rather the project just adopt squash merge commits 😅

I have a strong distaste for squashing commits as it loses valuable information on how changes are constructed, instead preferring people to contribute clean, atomic commits. However, that doesn't mean you have to go in and edit the message. That job is non-blocking and the lint output says to not stress too much over it.

[epage]

I think I prefer the original comment

[polarathene]

I have a strong distaste for squashing commits as it loses valuable information on how changes are constructed, instead preferring people to contribute clean, atomic commits.

That's alright, I encountered the same stance with the prior project maintainer back in 2023 where I explained why I disagree.

I find squash commits much more appropriate and cleaner in commit history personally, but only when you have the associated platform (GitHub) accessible. Since it's not uncommon for commits to make such references or delegate valuable information at the PR reviews or issues, the commit history often lacks that.

I do appreciate atomic commits, be that via rebase merge or the PR commit history being tidied up before a squash merge. The default merge commit strategy however is rather bad UX to navigate the history/log (which I often do via web UI along with it's equivalent git blame instead of cloning each project to inspect).

---

However, that doesn't mean you have to go in and edit the message. That job is non-blocking and the lint output says to not stress too much over it.

👍 Good to know, but then is that worth keeping? (I briefly raised the same question in 2023)

Due to the comment change request, I'll likely have to squash commit history anyway (one of the bonuses of merge commit for me as a maintainer is that contributors could easily iterate or I could apply suggestions myself and bundle that up without an interactive rebase, and I say this as probably one of the very few that properly leverages commit history on that project over the years 😅)

[polarathene]

Is this a better revision?

Suggested change

	// Append an extension that will be replaced when calling `set_extension()`:
	// Add a fake extension which will be replaced by `set_extension()` calls.
	// Prevents accidentally replacing a portion of the base filename with an existing `.` (ref: #306)

[epage]

If we're working to improve it, rather than preserve it (since it was sufficient)

Suggested change

	// Append an extension that will be replaced when calling `set_extension()`:
	// Preserve any extension-like text within the provided file stem (e.g. `file.local`) by appending a fake extension which will be replaced by `set_extension()` calls (e.g. `file.local.json`).

[polarathene]

Let me know your decision and I'll apply that.

Since there is very little in this PR, can I avoid the hassle of locally cloning this branch to perform a squash, and instead have the PR use a squash merge instead?

[polarathene]

Alternatively, I think using a conditional communicates the intention better 😅 (even though it's safe to just append always in our case)

Suggested change

	// Append an extension that will be replaced when calling `set_extension()`:
	let mut filename = filename;
	filename.as_mut_os_string().push(".ext_placeholder");
	let mut filename = filename;
	// Avoid accidental inference of an extension by appending a placeholder for `set_extension()` to replace instead (ref: #306)
	if filename.extension().is_some() {
	filename.as_mut_os_string().push(".placeholder");
	}

Comment with original PR reference of the issue for added context.

---

I'm not wanting to bike shed this though, so feel free to prefer your suggestion or the original and I'm happy to go with either of those too 👍 (I personally did not find the file.local example helpful, it's definitely not about existing extensions so much as it is a file name with one or more . getting it's final suffix replaced accidentally)

[epage]

I still prefer my version but can go either way on the location.

I personally did not find the file.local example helpful, it's definitely not about existing extensions so much as it is a file name with one or more . getting it's final suffix replaced accidentally

When you have a file stem with a . it can be mistaken for an extension which is what I was trying to convey in my text and with the example

[polarathene]

I still prefer my version but can go either way on the location.

"either way on location"? Sorry not sure what you meant by that.

Did you mean you would like your version, and are flexible with where it's located in the file? 🤔 (presumably because my last suggestion moved the comment under let mut filename instead of before it as it once was)

I'll assume that and go with your comment 👍

---

The remainder of this response is just sharing my opinion on why I think my version was simpler to grok (with minimal familiarity required to the reader).

When you have a file stem with a . it can be mistaken for an extension

It's technically not the file stem though, it's the full PathBuf 😅

If we used filename.file_stem() we'd get the stem, but with that same concern of the last . inferring an extension. So you're right in that at this point we want to think of filename as the file stem with the placeholder appended as the extension to be replaced.

The conditional statement added now better communicates this expectation of a preventative measure for an inferred extension? Which the associated comment provides additional clarity.

---

// Preserve any extension-like text within the provided file stem (e.g. `file.local`) by appending a fake extension which will be replaced by `set_extension()` calls (e.g. `file.local.json`).

This reads with more mental overhead to me at least 😅

• "file stem" file.local implicitly referring to filename
• file.local.json as example for output after a set_extension() call replaces the fake extension.

// Adding a dummy extension will make sure we will not override secondary extensions, i.e. "file.local"

This original comment didn't explain the "why" that well and I had to use git blame to lookup the commit/PR for extra context.

This one reads like myfile.tar as a 2nd extension to myfile.tar.gz, but I don't really think of extensions in a filename liberally using . either like my.file.name.here 😅

Accidental inference of an extension reads much more clearly (it's only a 2ndry extension in the context of intent to later append the config extensions via set_extension()).

[polarathene]

This has swapped out my comment for yours, with a slight modification to better convey the expected transformation for a file stem file.local. Hopefully a good compromise 😅

Suggested change

	// Append an extension that will be replaced when calling `set_extension()`:
	let mut filename = filename;
	filename.as_mut_os_string().push(".ext_placeholder");
	let mut filename = filename;
	// Preserve any extension-like text within the provided file stem by appending a fake extension
	// which will be replaced by `set_extension()` calls (e.g. `file.local.placeholder` => `file.local.json`)
	if filename.extension().is_some() {
	filename.as_mut_os_string().push(".placeholder");
	}

I'll commit and squash this, but I don't mind ammending the change again if you're not happy with it 👍

[epage]

Since there is very little in this PR, can I avoid the hassle of locally cloning this branch to perform a squash, and instead have the PR use a squash merge instead?

For regular contributors and those familiar enough with git, I would prefer they take care of these kinds of things than me squashing them. If nothing else, github leaves the field sticky and I can forget and accidentally squash something else.

[polarathene]

I did not realize I had created a branch on this repo 😬 My mistake, I had used the GH web editor and I'm used to the branch creation being done on my own fork but since I'm a collaborator it created the branch here 😓

Unfortunately, I'm getting permission errors when I try to push the rewritten history. I assume it's a limitation of collaborator permissions, or my local credentials lack the permissions for some reason.

---

I've pushed the fixed branch to my fork and opened a new PR here instead: #682

Will continue there, so the commentary since you last interacted on this PR would be this comment of mine which led to the changes applied at #682

Closing this PR and I'll attempt to delete the branch as cleanup.