fix: support ICustomFormatter via user-supplied IFormatProvider

[aradalvand]

Fixes #186

I don't believe this is a breaking change. Because the hand-written if clauses and whatnot that were previously there were doing, in my analysis, effectively a strict subset of what ScalarValue.Render does internally (and the latter does more, which is the crucial fix here, see #186).

P.S. There are two other approaches that you can see in the first and second commits. But I think delegating to ScalarValue.Render is the most correct one.

[nblumhardt]

Hi! Thanks for digging into this.

ScalarValue.Render() will (IIRC) lead to "quoted string"-style output and non-JSON structured data; a more tactical change might be needed here.

Would also be worth including a test to show where the behavior will differ with a custom formatter; there's a similar test in https://github.com/serilog/serilog/blob/dev/test/Serilog.Tests/Formatting/Display/MessageTemplateTextFormatterTests.cs#L168 (though it's a bit lean/minimal :-)).

[aradalvand]

Hi @nblumhardt! Thanks for checking this out!

ScalarValue.Render() will (IIRC) lead to "quoted string"-style output

You're right, only if the parameter is of type string though, which I think makes sense. Are you saying that's problematic? َUpdate: Ended up special-casing that.

non-JSON structured data

I think that's solved by virtue of the fact that we are still special-casing non-scalars and JSON-serializing them:

Would also be worth including a test to show where the behavior will differ with a custom formatter

Good point; I'll add a test for this.

[aradalvand]

I just added a couple of test cases (for testing CleanMessageTemplateFormatter in an isolated fashion as well as an "end-to-end" test), verifying that the ICustomFormatter works.

I also special-cased strings to make them unquoted, like before, as you seemed to prefer (and I think I agree).

Either way, I didn't touch the old tests, and all the tests (+ the new ones) are now passing, including the JSON test.

[nblumhardt]

Thanks for following up! Sorry about the slow reply.

Formatting through ScalarValue is something of a legacy path, so just making the minimal change here to support ICustomFormatter would be preferred. I can see that the existing code could use a bit of sanity-checking and clean-up :-) but I can take care of that down the path. The minimal change to support the added functionality would be a quicker and easier review/merge. Thanks again!

[aradalvand]

Hi @nblumhardt — no worries!
I just made another commit; let me know if the changeset is ideal now (no longer forwarding to ScalarValue).

[nblumhardt]

The deleted code here is all still required, without it, the format provider isn't passed through IFormattable.

[aradalvand]

I don't think so. Custom formatters often do this internally if they see fit.
Here's an example

public sealed class LogMessageFormatter(IFormatProvider? fallbackFormatter = null) : IFormatProvider, ICustomFormatter
{
	public static LogMessageFormatter WithInvariant { get; } = new(CultureInfo.InvariantCulture);

	public string Format(string? format, object? arg, IFormatProvider? _)
	{
		return arg switch
		{
			// NOTE: Print timestamps based on the host machine's timezone.
			Instant instant => instant
				.InZone(DateTimeZoneProviders.Tzdb.GetSystemDefault())
					.ToString("yyyy-MM-ddTHH:mm:sso<g>", formatProvider: fallbackFormatter), // NOTE: NodaTime's ZonedDateTime's format strings: https://nodatime.org/3.2.x/userguide/zoneddatetime-patterns

			// NOTE: Truncate GUIDs (because they're long AF), and only show the last 12 characters.
			IId id => id.Value.ToString()[^12..], // NOTE: We choose the last part (rather than the first bit, which is often more common when it comes to UUIDs), because we're using UUIDv7 which have their random bits at the end, the first ones signify the timestamp and are therefore prone to collision.

			// NOTE: Display the type's name rather than its fully-qualified name.
			Type type => type.SimpleName(),

			// NOTE: For anything else, if the object implements `IFormattable` (which means that it has a `.ToString()` overload that receives a format string), we want to use that. This is important in cases where the argument's format string is provided in the template (such as console template's `Timestamp`).
			IFormattable thing => thing.ToString(format, fallbackFormatter),

			_ => arg?.ToString() ?? "NULL",
		};
	}

	public object? GetFormat(Type? formatType)
	{
		return formatType == typeof(ICustomFormatter)
			? this
			: fallbackFormatter?.GetFormat(formatType);
	}
}

[nblumhardt]

If there's no custom formatter, though, we just fall through to output.Write(value) (no format provider used).