Skip to content

chore: file.rs - Remove add_dummy_extension() helper #678

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

Closed
wants to merge 2 commits into from
Closed

chore: file.rs - Remove add_dummy_extension() helper #678

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4009e0a
chore: `file.rs` - Remove `add_dummy_extension()` helper
polarathene Aug 15, 2025
353747a
Update src/file/source/file.rs
polarathene Aug 20, 2025
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
22 changes: 4 additions & 18 deletions src/file/source/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,10 @@ impl FileSourceFile {
)))
};
}
// Adding a dummy extension will make sure we will not override secondary extensions, i.e. "file.local"
// This will make the following set_extension function calls to append the extension.
Comment on lines -62 to -63
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer the original comment

let mut filename = add_dummy_extension(filename);

// Append an extension that will be replaced when calling `set_extension()`:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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`).

let mut filename = filename;
filename.as_mut_os_string().push(".ext_placeholder");
Copy link
Contributor Author

@polarathene polarathene Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍


match format_hint {
Some(format) => {
Expand Down Expand Up @@ -134,18 +135,3 @@ where
})
}
}

fn add_dummy_extension(mut filename: PathBuf) -> PathBuf {
match filename.extension() {
Some(extension) => {
let mut ext = extension.to_os_string();
ext.push(".");
ext.push("dummy");
filename.set_extension(ext);
}
None => {
filename.set_extension("dummy");
}
}
filename
}
Loading