Skip to content

Conversation

@snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Oct 4, 2025

Followup to #5955 (Instead of magic-value logic around Format-parameter, then change the official contract)

CaptureType.Stringify is kind of exotic, so not expecting much friction from the change (and legacy mode is available).

@snakefoot snakefoot added enhancement Improvement on existing feature breaking behavior change Same API, different result labels Oct 4, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 4, 2025

Walkthrough

Adjusts Stringify capture handling across template formatting, JSON layout, and value formatting to consistently quote stringified values at appropriate layers, refine null handling, and prevent duplicate quotes. Updates unit tests to reflect new quoting behavior and add coverage for Stringify across types.

Changes

Cohort / File(s) Summary
Message template formatting
src/NLog/Internal/LogMessageTemplateFormatter.cs
Refines RenderHolePositional and RenderHole: treats nulls explicitly, routes non-Stringify via serialization, and for Stringify appends quotes around formatted output; delegates to ValueFormatter accordingly.
JSON layout stringify quoting
src/NLog/Layouts/JSON/JsonLayout.cs
When CaptureType.Stringify, explicitly wraps formatted value with quotes before JSON escaping, ensuring stringified properties are quoted.
Value formatting behavior
src/NLog/MessageTemplates/ValueFormatter.cs
Tweaks Stringify quoting condition to add quotes only under legacy flag and when not duplicating trailing quote, reducing unintended double-quoting.
Unit tests: JSON layout
tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
Adds parameterized tests to validate JSON encoding of stringified event properties across multiple types and null.
Unit tests: renderer
tests/NLog.UnitTests/MessageTemplates/RendererTests.cs
Adds cases asserting "{$world}" outputs are quoted for null, numeric, and string inputs.
Unit tests: value formatter expectations
tests/NLog.UnitTests/MessageTemplates/ValueFormatterTest.cs
Updates expectations for Stringify results (removal or adjustment of surrounding quotes) and null handling to align with new behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as Application
  participant Logger as Logger
  participant Formatter as LogMessageTemplateFormatter
  participant VF as ValueFormatter

  App->>Logger: Log(message, args/properties)
  Logger->>Formatter: Format template holes
  alt Hole CaptureType = Stringify
    Formatter->>Formatter: RenderHolePositional(...)
    Formatter->>Formatter: RenderHole(value, Stringify)
    note right of Formatter: Adds opening quote
    Formatter->>VF: FormatValue(value, Stringify)
    VF-->>Formatter: Stringified content
    note right of Formatter: Adds closing quote
  else Other CaptureType
    Formatter->>VF: FormatValue(value, captureType)
    VF-->>Formatter: Serialized content or "NULL"
  end
  Formatter-->>Logger: Formatted message
  Logger-->>App: Output
Loading
sequenceDiagram
  autonumber
  participant App as Application
  participant Json as JsonLayout
  participant VF as ValueFormatter

  App->>Json: Render(LogEvent with properties)
  alt Property placeholder = {$name} (Stringify)
    Json->>Json: Append '"'
    Json->>VF: FormatValue(property, Stringify)
    VF-->>Json: Stringified content
    Json->>Json: Append '"'
    Json->>Json: PerformJsonEscapeIfNeeded(startPos)
  else Other properties
    Json->>VF: FormatValue(property, captureType)
    VF-->>Json: Content
  end
  Json-->>App: JSON string
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I nibble logs with tidy bites,
Quoting strings on starry nights;
Nulls now whisper, neat and small,
Curly braces, I hug them all.
Tests hop by—thump-thump, they cheer—
“Stringify’s clear!” with floppy ear.
Carrots for code, the path is dear.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% 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 succinctly identifies the key change to ValueFormatter behavior by specifying that quotes are skipped for CaptureType.Stringify unless legacy mode is enabled, which accurately reflects the main purpose of the pull request.
Description Check ✅ Passed The description directly references the change in CaptureType.Stringify behavior and the intent to adjust quoting logic following a previous issue, which aligns with the modifications in ValueFormatter and related tests. It is clearly related to the changeset and provides context for the behavioral change without being off-topic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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 closed this Oct 4, 2025
@snakefoot snakefoot reopened this Oct 4, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 4, 2025

@snakefoot snakefoot enabled auto-merge (squash) October 4, 2025 09:25
@snakefoot snakefoot disabled auto-merge October 4, 2025 09:26
@snakefoot snakefoot merged commit afb553c into NLog:dev Oct 4, 2025
5 of 6 checks passed
@snakefoot snakefoot added this to the 6.0.5 milestone Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking behavior change Same API, different result enhancement Improvement on existing feature size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant