FileTarget - Archive Cleanup sort filenames using natural ordering #6056
Conversation
WalkthroughReplaces simple string comparison with a new natural string comparer in BaseFileArchiveHandler to order archive filenames when creation dates tie, and updates FileTarget unit tests to accept and pass a new maxArchiveFiles parameter to GenerateArchives. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bdc4ddb to
6abc135
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(1 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (1)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
146-160: Natural filename sort logic looks sound for archive cleanup use caseThe switch to
NaturalStringComparerwhen dates tie (Line 154) correctly addresses numeric ordering of names like..._99vs..._100, avoiding lexicographic mis-ordering that could cause high sequence numbers to be deleted first. The implementation:
- Compares shared numeric blocks by length (after skipping leading zeros) and then digit-by-digit.
- Falls back to case-insensitive ordinal comparison for non-digit segments.
- Uses total length as a final tie-breaker, which is fine for file names.
I don’t see correctness issues for archive file patterns used here, and the behavior on edge cases (leading zeros, differing digit widths) is reasonable. No changes needed from my side.
Also applies to: 162-235
| [Theory] | ||
| [InlineData("##", 0, "00")] | ||
| [InlineData("##", 1, "01")] | ||
| [InlineData("#", 20, "20")] | ||
| [InlineData(0, "00", 0)] | ||
| [InlineData(1, "01", 0)] | ||
| [InlineData(20, "20", 0)] | ||
| [InlineData(101, "101", 10)] | ||
| public void FileTarget_WithDateAndSequenceArchiveNumbering_ShouldPadSequenceNumberInArchiveFileName( | ||
| string placeHolderSharps, int sequenceNumber, string expectedSequenceInArchiveFileName) | ||
| int sequenceNumber, string expectedSequenceInArchiveFileName, int maxArchiveFiles) | ||
| { | ||
| string tempDir = Path.Combine(Path.GetTempPath(), "nlog_" + Guid.NewGuid().ToString()); | ||
| const string archiveDateFormat = "yyyy-MM-dd"; | ||
| string archiveFileName = Path.Combine(tempDir, $"{{{placeHolderSharps}}}.log"); | ||
| string archiveFileName = Path.Combine(tempDir, $"{{##}}.log"); | ||
| string expectedArchiveFullName = | ||
| $"{tempDir}/{DateTime.Now.ToString(archiveDateFormat)}_{expectedSequenceInArchiveFileName}.log"; | ||
|
|
||
| GenerateArchives(count: sequenceNumber + 2, archiveDateFormat: archiveDateFormat, | ||
| archiveFileName: archiveFileName); | ||
| archiveFileName: archiveFileName, maxArchiveFiles); | ||
| bool resultArchiveWithExpectedNameExists = File.Exists(expectedArchiveFullName); |
There was a problem hiding this comment.
Fix GenerateArchives call: positional arg after named args will not compile
The new GenerateArchives signature and wiring of maxArchiveFiles look good, but this call mixes named and positional arguments:
GenerateArchives(count: sequenceNumber + 2, archiveDateFormat: archiveDateFormat,
archiveFileName: archiveFileName, maxArchiveFiles);In C#, once you start using named arguments, all subsequent arguments must also be named; this will be a compile error on the supported targets.
Recommend making the last argument named as well (or reverting to all-positional):
- GenerateArchives(count: sequenceNumber + 2, archiveDateFormat: archiveDateFormat,
- archiveFileName: archiveFileName, maxArchiveFiles);
+ GenerateArchives(
+ count: sequenceNumber + 2,
+ archiveDateFormat: archiveDateFormat,
+ archiveFileName: archiveFileName,
+ maxArchiveFiles: maxArchiveFiles);The rest of the changes (extra test parameter and the optional maxArchiveFiles in GenerateArchives) nicely exercise the interaction between sequence numbering and archive cleanup.
Also applies to: 3477-3493
🤖 Prompt for AI Agents
In tests/NLog.UnitTests/Targets/FileTargetTests.cs around lines 3438 to 3454
(and similarly at 3477-3493), the GenerateArchives call mixes named and
positional arguments which will not compile; make the final argument named as
well (e.g., maxArchiveFiles: maxArchiveFiles) or convert all arguments to
positional so all arguments follow the same style; update both occurrences
accordingly.
6abc135 to
c413964
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
162-238: Natural string comparer logic looks sound for numeric archive suffixes; consider small clarity/test follow-upsThe implementation correctly handles the core natural-sort cases for archive filenames:
- Numeric blocks are compared by significant length after trimming leading zeros, then digit-by-digit, which yields the expected order for
..._9,..._10,..._99,..._100, etc.- Non-digit segments are compared case-insensitively and deterministically.
- When there are no impactful numeric differences (or only fully equal numeric blocks), the final length comparison gives a stable tie-breaker.
Two optional polish ideas:
- Add a brief comment explaining why equal-length strings are delegated directly to
string.Compare(fixed-width numeric segments + constant suffixes means ordinal is already numeric-correct).- Back this helper with a focused unit-test set that exercises edge cases (e.g., mixed leading zeros, multiple numeric blocks,
01vs1, end-of-string numeric blocks) so future refactors don’t accidentally regress the ordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(1 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/NLog.UnitTests/Targets/FileTargetTests.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (1)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
146-159: Switching toNaturalStringComparerfor same-day files aligns with the archive cleanup goalUsing
NaturalStringCompareronly whenArchiveSequenceNumberis unavailable and the dates match keeps the existing primary sort key (sequence number) intact while fixing the lexicographic-vs-numeric ordering problem for filenames like..._99vs..._100. The change is localized, deterministic, and consistent with the delete-from-oldest behavior inCleanupFiles.
c413964 to
2058050
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
3452-3453: Fix compilation error: positional argument after named arguments.This issue was previously identified in past review comments. The call mixes named and positional arguments, which is a compilation error in C#. Once you use named arguments, all subsequent arguments must also be named.
Apply this diff:
- GenerateArchives(count: sequenceNumber + 2, archiveDateFormat: archiveDateFormat, - archiveFileName: archiveFileName, maxArchiveFiles); + GenerateArchives( + count: sequenceNumber + 2, + archiveDateFormat: archiveDateFormat, + archiveFileName: archiveFileName, + maxArchiveFiles: maxArchiveFiles);
🧹 Nitpick comments (1)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
162-164: Clarify version reference in XML comment.The comment mentions "NET10" which is ambiguous. Consider clarifying as ".NET 10.0" or ".NET 10" for better readability.
Apply this diff:
/// <summary> -/// With NET10 there is CompareOptions.NumericOrdering (But not supported on all operating systems) +/// With .NET 10.0 there is CompareOptions.NumericOrdering (But not supported on all operating systems) /// </summary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(1 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
src/NLog/Targets/FileTarget.cs (2)
DateTime(837-855)DateTime(865-873)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (4)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (2)
152-155: LGTM! Natural sort integration is correctly scoped.The replacement of
StringComparer.OrdinalIgnoreCasewithNaturalStringCompareris applied only when creation dates are equal, preserving the existing sort hierarchy (sequence number → date → filename). This ensures that archive files likefile_99.txtandfile_100.txtwill be ordered correctly even when the sequence number exceeds the formatted digit width.
165-238: Approve the natural string comparer implementation.The implementation correctly handles natural sorting for archive filenames:
- Numeric block comparison: Compares by length first (longer numbers are greater), then digit-by-digit for same-length numbers, which correctly orders sequences like "99" < "100"
- Leading zero handling: Properly skips leading zeros so "001" and "1" are treated as equal numerically
- Prefix optimization: Skips the common prefix before comparing, improving efficiency
- Fallback logic: Uses length comparison as the final tiebreaker
This addresses issue #6055 where archive sequence numbers exceeding the formatted digit width (e.g., 99 → 100) were not sorted correctly.
tests/NLog.UnitTests/Targets/FileTargetTests.cs (2)
3438-3444: Test parameterization updated appropriately.The test signature changes support testing the natural sort fix for archive cleanup:
- Replaced
placeHolderSharpswith direct use of{##}pattern (line 3448)- Added
maxArchiveFilesparameter to verify archive cleanup behavior with both unlimited (0) and limited (10) scenarios- Test cases now validate the scenario from issue #6055 where sequence numbers exceed formatted width (line 3442:
sequenceNumber=101)
3477-3493: LGTM! GenerateArchives helper method updated correctly.The addition of the optional
maxArchiveFilesparameter with appropriate conditional logic is well-designed:
- Default value of 0 allows existing test calls to work unchanged (e.g., line 3471)
- Only sets
MaxArchiveFileswhen> 0, which correctly handles the unlimited-archives case- Enables testing of archive cleanup behavior with the new natural sort implementation
|
Resolves #6055