src/NLog/MessageTemplates/ValueFormatter.cs (1)

121-126: Stringify quoting semantics changed; confirm desired behavior for positional vs named holes

Using includeQuotes = (format is null || _legacyStringQuotes) means:

• Named holes with Stringify (format == null) will be quoted by default.
• Positional holes now pass empty string (not null) from LogMessageTemplateFormatter, so they won’t be quoted unless legacy flag is on.

If you want ":l" to also suppress quotes for Stringify, consider this tweak.

- bool includeQuotes = format is null || _legacyStringQuotes;
+ bool includeQuotes = (format is null || _legacyStringQuotes)
+                      && !string.Equals(format, LiteralFormatSymbol, StringComparison.Ordinal);

Please verify behavior for:

• "{0}" with a string (no quotes), "{$0}" with a string (quotes), and "{$0:l}" (no quotes).

src/NLog/Internal/LogMessageTemplateFormatter.cs (1)

282-289: Positional holes now flow through IValueFormatter (Serialize/Stringify); subtle quoting shift

Pros:

• Unifies formatting pipeline and enables IValueFormatter overrides (as tested).
• Consistent handling across positional/named holes.

Gotchas:

• Normal capture for positional holes now uses Stringify, which may quote strings if legacy flag is on (differs from old string.Format semantics). This seems intentional; please confirm legacy scenarios remain acceptable.

Consider adding a comment here explaining the empty format for Stringify is intentional to avoid quotes for positional placeholders.

tests/NLog.UnitTests/MessageTemplates/RendererTests.cs (3)

109-124: Great targeted test for overriding IValueFormatter (DateTime default format)

Solid validation of the new pipeline. Consider also testing:

• Named hole "{time}" (non-positional) to confirm override applies there too.
• "{$0}" and "{$time}" to check Stringify + override interplay and quoting.

I can add these test cases if you want.

---

148-154: RenderAndTest overload injection is clean

Optional: use a shared LogFactory across related tests to reduce overhead if many future tests will inject services.

---

156-178: OverrideValueFormatter implementation is correct and minimal

Minor nits (optional):

• Accept nullable parameters to mirror IValueFormatter nullability.
• Consider restricting the override to CaptureType.Stringify/Normal if Serialize should bypass (policy-dependent).

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/NLog/Internal/LogMessageTemplateFormatter.cs (1)

299-306: Delegate null handling in RenderHole to ValueFormatter for consistency

ValueFormatter.FormatValue already handles null according to CaptureType (JSON null, stringify "", default "NULL") and will pick up any custom IValueFormatter. Remove the inline null check so all cases go through the same logic.

• File: src/NLog/Internal/LogMessageTemplateFormatter.cs
Method: RenderHole(StringBuilder sb, CaptureType captureType, string? holeFormat, IFormatProvider? formatProvider, object? value) (lines ~299–306)

Suggested diff:

-        if (value is null)
-        {
-            sb.Append("NULL");
-        }
-        else
-        {
-            ValueFormatter.FormatValue(value, holeFormat, captureType, formatProvider, sb);
-        }
+        // Delegate null handling and all formatting to ValueFormatter.FormatValue
+        ValueFormatter.FormatValue(value, holeFormat, captureType, formatProvider, sb);

Please also add or update unit tests to verify:

• CaptureType.Serialize with null → null
• default capture with null → "NULL"
• CaptureType.Stringify with null → ""
• custom IValueFormatter overrides null formatting as expected

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/NLog/Internal/LogMessageTemplateFormatter.cs (3)

288-289: Confirm intent of using empty string for hole.Format in stringify path

You pass hole.Format ?? string.Empty for Stringify, but pass hole.Format as-is for Serialize. If ValueFormatter distinguishes null vs empty, this could diverge from named-hole behavior (where null is forwarded). Please confirm this is intentional to preserve string.Format-like behavior (e.g., no quotes for strings) for positional holes, and consider adding tests to lock this in:

• "{0}" with string/date/time/enum (no format) should match string.Format output.
• "{0:O}" and other formats for DateTime/decimal should honor formatProvider.

---

297-307: Ensure NULL rendering remains consistent with the rest of the pipeline

RenderHole appends "NULL" for nulls. Since positional holes now route through this method, null output is unified. Verify that this casing/text matches expectations across all sinks/formatters. If there’s a central setting or constant for null text in ValueFormatter, consider using it for consistency.

---

321-325: Typo: holeFormatVaue -> holeFormatValue

Minor readability fix.

-                    string holeFormatVaue = sb.ToString(holeStartPosition, holeWidth);
+                    string holeFormatValue = sb.ToString(holeStartPosition, holeWidth);
-                    sb.Append(holeFormatVaue);
+                    sb.Append(holeFormatValue);

tests/NLog.UnitTests/MessageTemplates/ValueFormatterTest.cs (2)

137-140: Strengthen assertions to robustly detect unintended flattening.

Current NotEqual checks could pass even if output is a flat list with different spacing. Prefer a regex-based assertion that rejects a plain comma-separated list of numbers.

-            Assert.NotEqual("1,2,3,4,5,6,7", builder.ToString());
+            Assert.DoesNotMatch(@"^\s*\d+(,\s*\d+)*\s*$", builder.ToString());

-            Assert.NotEqual("1,2", builder.ToString());
+            Assert.DoesNotMatch(@"^\s*\d+(,\s*\d+)*\s*$", builder.ToString());

Also applies to: 147-150

---

86-99: Consider adding a positive test for non-null format strings.

Since callers now pass null when no format is intended, add a dedicated test that passes a non-null format (e.g., numeric "D4" or DateTime "yyyy-MM-dd") to validate that explicit formats are still honored.

Example you could add:

[Fact]
public void FormatValue_AppliesExplicitFormatString()
{
    var value = 12;
    var builder = new StringBuilder();
    var ok = CreateValueFormatter().FormatValue(value, "D4", CaptureType.Normal, CultureInfo.InvariantCulture, builder);
    Assert.True(ok);
    Assert.Equal("0012", builder.ToString());
}

Happy to provide a PR-ready patch if helpful.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro