Make baseName consistent with C++17 filesystem::path::stem#281
Make baseName consistent with C++17 filesystem::path::stem#281
Conversation
baseName(false) stripped from the first dot instead of the last, producing incorrect results for multi-extension filenames such as archive.tar.gz (yielded "archive" instead of "archive.tar"). This is inconsistent with C++17 std::filesystem::path::stem and with eckit's own extension() method which already uses find_last_of. Fixes: ECKIT-415 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes LocalPathName::baseName(false) so it strips only the final extension (matching C++17 std::filesystem::path::stem) and adds regression tests for multi-dot and hidden filenames (ECKIT-415).
Changes:
- Update
LocalPathName::baseName(false)to strip from the last.(not the first). - Expand filesystem tests to cover multi-extension filenames and hidden files.
- Clarify
baseName()documentation about stripping only the final extension.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/eckit/filesystem/LocalPathName.cc |
Changes baseName(false) to use the last dot when stripping the extension. |
src/eckit/filesystem/LocalPathName.h |
Updates the baseName() parameter documentation to reflect “final extension only”. |
tests/filesystem/test_localpathname.cc |
Adds new test cases for multi-dot, double-dot, and hidden filenames. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| EXPECT(LocalPathName("file..txt").baseName(false) == "file."); | ||
| EXPECT(LocalPathName("file..txt").baseName(true) == "file..txt"); | ||
|
|
There was a problem hiding this comment.
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.
| // Trailing-dot filenames: strip only the final extension (empty extension case) | |
| EXPECT(LocalPathName("fred..").baseName(false) == "fred."); | |
| EXPECT(LocalPathName("fred..").baseName(true) == "fred.."); |
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| /// Base name of the path | ||
| /// @param ext if false the extension is stripped | ||
| /// @param ext if false only the final extension is stripped |
There was a problem hiding this comment.
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.
| /// @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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #281 +/- ##
========================================
Coverage 66.31% 66.31%
========================================
Files 1125 1125
Lines 57602 57605 +3
Branches 4403 4401 -2
========================================
+ Hits 38199 38201 +2
- Misses 19403 19404 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
baseName(false) stripped from the first dot instead of the last, producing incorrect results for multi-extension filenames such as archive.tar.gz (yielded "archive" instead of "archive.tar"). This is inconsistent with C++17 std::filesystem::path::stem and with eckit's own extension() method which already uses find_last_of.
Fixes: ECKIT-415
Contributor Declaration
By opening this pull request, I affirm the following:
🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/eckit/pull-requests/PR-281