Two bugfixes for logging generator#51963
Conversation
|
Tagging subscribers to this area: @maryamariyan Issue Details
cc: @geeknoid
|
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Just so the generated file name would become LoggerMessage.cs instead of LoggerMessageGenerator.cs
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
gfoidl
left a comment
There was a problem hiding this comment.
One (minor) suggestion, otherwise LGTM.
Thanks!
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.
eerhardt
left a comment
There was a problem hiding this comment.
Looks good. Just a nit and a thought.
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