Add more overloads to LoggerMessage.Define #296
Conversation
|
Hi @pakrym, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| /// <param name="eventId">The event id</param> | ||
| /// <param name="formatString">The named format string</param> | ||
| /// <returns>A delegate which when invoked creates a log message.</returns> | ||
| public static Action<ILogger, Exception> Define(LogLevel logLevel, int eventId, string formatString) |
There was a problem hiding this comment.
This extension should not be required as the intention of the extensions in this file is to optimize for the scenarios where we pass parameters to the format string (where as here we do not pass any)...
Example:
public static void OrderPlaced(this ILogger logger)
{
if (logger.IsEnabled(LogLevel.Information)
{
logger.LogInformation(eventId: 10, "Order placed");
}
}
There was a problem hiding this comment.
True, but we get LoggingExtensions with two usage patterns inside which doesn't look good.
There was a problem hiding this comment.
@kichalla I said it was ok 😄
There was a problem hiding this comment.
@kichalla in this particular case it's more about consistency and not any actual perf concern.
|
This addresses this issue: #272 |
|
|
||
| public override string ToString() => _formatter.Format(ToArray()); | ||
| } | ||
|
|
|
Is it possible to add at least a basic test for these? |
|
@Eilon will do |
|
🆙 📅 |
| @@ -162,6 +163,48 @@ public void LogScope_WithThreeParameters() | |||
| actualLogValues.ToString()); | |||
| } | |||
|
|
|||
| [Theory] | |||
| [MemberData(nameof(LogMessagesData))] | |||
|
|
|||
|
|
after code formatting.
pakrym
force-pushed
the
pakrym/more-overloads
branch
from
9a3ba74 to
646bb6e
Compare
@lodejard