Skip to content

Make baseName consistent with C++17 filesystem::path::stem #281

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

Open
Ozaq wants to merge 1 commit into
base: develop
Choose a base branch
from
+21 −14
Open

Make baseName consistent with C++17 filesystem::path::stem #281

Show file tree
Hide file tree
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: 3 additions & 13 deletions src/eckit/filesystem/LocalPathName.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,19 +167,9 @@ LocalPathName LocalPathName::baseName(bool ext) const {

std::string s(path_.c_str() + n + 1);
if (!ext) {
n = -1;
i = 0;
q = s.c_str();
while (*q) {
if (*q == '.') {
n = i;
break;
}
q++;
i++;
}
if (n >= 0) {
s.resize(n);
const auto lastDot = s.find_last_of('.');
if (lastDot != std::string::npos && lastDot != 0) {
s.resize(lastDot);
}
}
Comment on lines 168 to 174
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

baseName(false) now treats a leading '.' as not being an extension separator (e.g. ".hidden" stays ".hidden"), but extension() still returns the substring from the last '.', which makes .hidden report an extension of ".hidden". This is inconsistent with the new semantics (and with std::filesystem), and can lead to surprising results if callers split/recombine using baseName(false) + extension(). Consider updating extension() to return "" when the only dot is a leading dot (and adding/adjusting tests accordingly), or clearly documenting that extension() treats leading dots differently.

Copilot uses AI. Check for mistakes.

Expand Down
2 changes: 1 addition & 1 deletion src/eckit/filesystem/LocalPathName.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class LocalPathName {
std::string clusterName() const;

/// Base name of the path
/// @param ext if false the extension is stripped
/// @param ext if false only the final extension is stripped
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The updated baseName(false) behavior also special-cases hidden files (leading dot is not treated as an extension separator). The header comment only mentions stripping the final extension; please document this leading-dot rule as part of the contract so callers understand what happens for names like .hidden and .hidden.tar.gz.

Suggested change
/// @param ext if false only the final extension is stripped
/// @param ext if false only the final extension is stripped. The leading '.' of a
/// hidden file is not treated as an extension separator, e.g. `.hidden` stays
/// `.hidden`, and `.hidden.tar.gz` becomes `.hidden.tar` when ext is false.

Copilot uses AI. Check for mistakes.
/// @return the name part of the path
LocalPathName baseName(bool ext = true) const;

Expand Down
17 changes: 17 additions & 0 deletions tests/filesystem/test_localpathname.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,23 @@ CASE("Extract basename") {
LocalPathName p("/a/made/up/path/that/does/not/exist/file.ok");
EXPECT(p.baseName(false) == "file");
EXPECT(p.baseName(true) == "file.ok");

// ECKIT-415: multi-dot filenames should strip only the final extension
EXPECT(LocalPathName("/path/archive.tar.gz").baseName(false) == "archive.tar");
EXPECT(LocalPathName("/path/archive.tar.gz").baseName(true) == "archive.tar.gz");

EXPECT(LocalPathName("file..txt").baseName(false) == "file.");
EXPECT(LocalPathName("file..txt").baseName(true) == "file..txt");

Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

Tests cover multi-extension names and hidden files, but there isn’t a regression assertion for the related trailing-dot case (e.g. "fred.." should strip only the final '.' and become "fred."). Adding this helps lock in the intended “strip only the final extension” behavior for edge cases with empty extensions.

Suggested change
// Trailing-dot filenames: strip only the final extension (empty extension case)
EXPECT(LocalPathName("fred..").baseName(false) == "fred.");
EXPECT(LocalPathName("fred..").baseName(true) == "fred..");

Copilot uses AI. Check for mistakes.
// Hidden files (leading dot is not an extension separator)
EXPECT(LocalPathName("/path/.hidden").baseName(false) == ".hidden");
EXPECT(LocalPathName("/path/.hidden").baseName(true) == ".hidden");
EXPECT(LocalPathName("/path/.hidden.tar.gz").baseName(false) == ".hidden.tar");
EXPECT(LocalPathName("/path/.hidden.tar.gz").baseName(true) == ".hidden.tar.gz");

// Directory with dots in the path should not affect baseName
EXPECT(LocalPathName("/path/with.dot/to/file.ext").baseName(false) == "file");
EXPECT(LocalPathName("/path/with.dot/to/file.ext").baseName(true) == "file.ext");
}

CASE("Extract extension from Path") {
Expand Down
Loading