Skip to content

FileTarget - Render ArchiveSuffixFormat using NLog DefaultCultureInfo #6039

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

Jump to bottom
Merged
snakefoot merged 1 commit into from
Nov 7, 2025
Merged

FileTarget - Render ArchiveSuffixFormat using NLog DefaultCultureInfo #6039

snakefoot merged 1 commit into from
Nov 7, 2025
+4 −2

Conversation

@snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Nov 7, 2025

Followup to #6027

@snakefoot snakefoot added enhancement Improvement on existing feature file-target file-archiving Issues with archiving with the file target labels Nov 7, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 7, 2025 •
edited
Loading

Walkthrough

FileTarget's BuildFullFilePath method now applies culture-aware formatting to archive suffix generation by retrieving CultureInfo from the logging configuration and applying it consistently to both the primary ArchiveSuffixFormat path and the legacy sequence suffix fallback path.

Changes

Cohort / File(s) Summary
Culture-aware archive suffix formatting
src/NLog/Targets/FileTarget.cs		 Modified BuildFullFilePath to capture CultureInfo from LoggingConfiguration or LogManager and apply it to string.Format calls for ArchiveSuffixFormat and legacy sequence suffix formatting, ensuring consistent culture-aware formatting across both code paths

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single file modification with localized, straightforward logic changes
  • Internal formatting adjustments without public API or method signature changes
  • Culture awareness applied consistently across two related formatting paths; verify correct fallback precedence (LoggingConfiguration?.LogFactory?.DefaultCultureInfo → LogManager.LogFactory.DefaultCultureInfo)

Poem

🐰 A format aware, a culture's care,
Through archive paths both old and new,
The suffix dances, cultures dance as
Logging blooms in global hue! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: making FileTarget render ArchiveFileName using NLog's DefaultCultureInfo for culture-aware formatting.
Description check ✅ Passed The description references PR #6027 as context, which is related to the changeset focused on culture-aware archive file name formatting.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c64c6ae and 0cee5e0.

📒 Files selected for processing (1)
  • src/NLog/Targets/FileTarget.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Targets/FileTarget.cs (2)
src/NLog/Config/LoggingConfiguration.cs (4)
  • LoggingConfiguration (53-1086)
  • LoggingConfiguration (70-73)
  • LoggingConfiguration (78-83)
  • LoggingConfiguration (485-488)
src/NLog/LogFactory.cs (5)
  • LogFactory (53-1275)
  • LogFactory (123-126)
  • LogFactory (147-155)
  • LogFactory (424-429)
  • LogFactory (1042-1066)
🔇 Additional comments (1)
src/NLog/Targets/FileTarget.cs (1)

1101-1112: LGTM! Culture-aware formatting properly implemented.

The changes correctly apply DefaultCultureInfo to both the primary ArchiveSuffixFormat path and the legacy sequence suffix fallback, ensuring consistent culture-aware formatting for archive file names. The defensive fallback chain (LoggingConfiguration?.LogFactory?.DefaultCultureInfo ?? LogManager.LogFactory.DefaultCultureInfo) is appropriate, and null handling is correct since string.Format accepts null IFormatProvider and falls back to CultureInfo.CurrentCulture.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@snakefoot snakefoot added this to the 6.0.6 milestone Nov 7, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 7, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
66.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@snakefoot snakefoot merged commit 2b1b40f into NLog:dev Nov 7, 2025
5 of 6 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Nov 8, 2025
Merged
@snakefoot snakefoot changed the title FileTarget - Render ArchiveFileName using NLog DefaultCultureInfo FileTarget - Render ArchiveSuffixFormat using NLog DefaultCultureInfo Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement Improvement on existing feature file-archiving Issues with archiving with the file target file-target size/XS

Projects

None yet

Milestone

6.0.6

Development

Successfully merging this pull request may close these issues.

1 participant

@snakefoot