Two bugfixes for logging generator #51963
Conversation
|
Tagging subscribers to this area: @maryamariyan Issue Details
cc: @geeknoid
|
maryamariyan
changed the title
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
| @@ -39,7 +39,7 @@ public void Execute(GeneratorExecutionContext context) | |||
| var e = new Emitter(); | |||
| string result = e.Emit(logClasses, context.CancellationToken); | |||
|
|
|||
| context.AddSource(nameof(LoggerMessageGenerator), SourceText.From(result, Encoding.UTF8)); | |||
| context.AddSource("LoggerMessage", SourceText.From(result, Encoding.UTF8)); | |||
There was a problem hiding this comment.
Just so the generated file name would become LoggerMessage.cs instead of LoggerMessageGenerator.cs
| @@ -443,63 +445,56 @@ static string GetLogLevel(LoggerMethod lm) | |||
| } | |||
| } | |||
|
|
|||
| private void GenEnumerationHelper(LoggerClass lc) | |||
There was a problem hiding this comment.
The enumeration helper is either generated (only once), or not at all into __LoggerMessageGenerator
- bugfix: up to 6 params, use LoggerMessage.Define - bugfix: "__Enumerate" method was missing for IEnumerable arguments - change "__Enumerate" method: made part of a utility method, generated once in __LoggerMessageGenerator
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
| [{_generatedCodeAttribute}] | ||
| private static string __Enumerate(global::System.Collections.IEnumerable? enumerable) | ||
| [{_generatedCodeAttribute}] | ||
| internal static class __LoggerMessageGenerator |
There was a problem hiding this comment.
Just thinking out loud here, but I almost wonder if this deserves to be a public API in our library instead of generating this code into the user's project.
I started by thinking "is putting this in the global namespace the right thing to do?" And then that quickly led to "why are we generating this code when it never changes based on the user's types?"
There was a problem hiding this comment.
In the very first API review proposal (Refer to comment #36022 (comment)), we proposed this static ArgumentFormatter.Enumerate(..) helper method which we said would only be used in the generated code.
The consensus back then was for such cases to just generate them if/when needed.
ghost
locked as resolved and limited conversation to collaborators
LoggerMessage.Define__Enumeratemethod was missing for IEnumerable arguments__Enumeratemethod: made part of a utility method, generating only once in__LoggerMessageGeneratorcc: @geeknoid, @gfoidl
Fixes: #51917, #51965