Skip to content

Fix Logging formatting with multiple collections #106213

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

Closed
wants to merge 1 commit into from
Closed

Fix Logging formatting with multiple collections #106213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,21 +197,18 @@ internal string Format<TArg0, TArg1>(TArg0 arg0, TArg1 arg1)
{
object? arg0String = null, arg1String = null;
return
!TryFormatArgumentIfNullOrEnumerable(arg0, ref arg0String) &&
!TryFormatArgumentIfNullOrEnumerable(arg1, ref arg1String) ?
string.Format(CultureInfo.InvariantCulture, _format, arg0, arg1) :
string.Format(CultureInfo.InvariantCulture, _format, arg0String ?? arg0, arg1String ?? arg1);
TryFormatArgumentIfNullOrEnumerable(arg0, ref arg0String) | TryFormatArgumentIfNullOrEnumerable(arg1, ref arg1String) ?
Copy link
Member

@ericstj ericstj Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make TryFormatArgumentIfNullOrEnumerable set stringValue to null when it returns false just to be explicit? I wonder if there is really any reason to check the return value from the Try method at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make TryFormatArgumentIfNullOrEnumerable set stringValue to null when it returns false just to be explicit?

I think it is written this way to avoid forcing initializing the stringValue inside the method. But we initlaize anyway the local variables before calling the Try methods. We can apply your suggestion to be consistent with other Try patterns.

I wonder if there is really any reason to check the return value from the Try method at all.

The only benefit of the checking is to avoid rechecking the args again when calling string.Format. I am not seeing is a big deal eitherway.

string.Format(CultureInfo.InvariantCulture, _format, arg0String ?? arg0, arg1String ?? arg1) :
string.Format(CultureInfo.InvariantCulture, _format, arg0, arg1);
}

internal string Format<TArg0, TArg1, TArg2>(TArg0 arg0, TArg1 arg1, TArg2 arg2)
{
object? arg0String = null, arg1String = null, arg2String = null;
return
!TryFormatArgumentIfNullOrEnumerable(arg0, ref arg0String) &&
!TryFormatArgumentIfNullOrEnumerable(arg1, ref arg1String) &&
!TryFormatArgumentIfNullOrEnumerable(arg2, ref arg2String) ?
string.Format(CultureInfo.InvariantCulture, _format, arg0, arg1, arg2) :
string.Format(CultureInfo.InvariantCulture, _format, arg0String ?? arg0, arg1String ?? arg1, arg2String ?? arg2);
TryFormatArgumentIfNullOrEnumerable(arg0, ref arg0String) | TryFormatArgumentIfNullOrEnumerable(arg1, ref arg1String) | TryFormatArgumentIfNullOrEnumerable(arg2, ref arg2String) ?
string.Format(CultureInfo.InvariantCulture, _format, arg0String ?? arg0, arg1String ?? arg1, arg2String ?? arg2):
string.Format(CultureInfo.InvariantCulture, _format, arg0, arg1, arg2);
}
#else
internal string Format(object? arg0) =>
Expand Down
25 changes: 25 additions & 0 deletions ...ons.Logging.Console/tests/Microsoft.Extensions.Logging.Console.Tests/ConsoleLoggerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1409,6 +1409,31 @@ public void ConsoleLoggerOptions_IncludeScopes_IsReadFromLoggingConfiguration()
Assert.True(formatter.FormatterOptions.IncludeScopes);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
public void LogMultipleArrays()
{
// Arrange
using var t = SetUp();
var logger = t.Logger;
var sink = t.Sink;

var define1 = LoggerMessage.Define<string[], string[]>(LogLevel.Information, new EventId(), "Log: {Array1} and {Array2}");
var define2 = LoggerMessage.Define<int, string[], string[]>(LogLevel.Information, new EventId(), "Log {Number}: {Array1} and {Array2}");

// Act
define1(logger, ["a", "b", "c"], ["d", "e", "f"], null);
define2(logger, 30, ["a", "b", "c"], ["d", "e", "f"], null);

var expectedMessage1 = $"{CreateHeader(ConsoleLoggerFormat.Default)}{Environment.NewLine}{_paddingString}Log: a, b, c and d, e, f{Environment.NewLine}";
var expectedMessage2 = $"{CreateHeader(ConsoleLoggerFormat.Default)}{Environment.NewLine}{_paddingString}Log 30: a, b, c and d, e, f{Environment.NewLine}";

Assert.Equal(4, sink.Writes.Count);
Assert.Equal("info", sink.Writes[0].Message);
Assert.Equal(expectedMessage1, sink.Writes[1].Message);
Assert.Equal("info", sink.Writes[2].Message);
Assert.Equal(expectedMessage2, sink.Writes[3].Message);
}

public static TheoryData<ConsoleLoggerFormat, LogLevel> FormatsAndLevels
{
get
Expand Down
Loading