Fix handling of culture in logging generator for > 6 parameters#117009
Fix handling of culture in logging generator for > 6 parameters#117009stephentoub merged 3 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
Ensures log messages with more than six parameters use invariant culture formatting.
- Adds
M4andM5test methods inTemplateTestExtensionsfor single- and multi-parameter logging. - Introduces a unit test (
TemplateTests_UsesInvariantCulture) to verify formatting under a non-defaultCurrentCulture. - Updates the generator to detect and use
string.Create(or fallback toFormattableString.Invariant) for interpolated strings in generated code.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| TemplateTestExtensions.cs | Added M4 and M5 partial methods to test 1 and 7 parameters. |
| LoggerMessageGeneratedCodeTests.cs | Removed unused using, added using System.Globalization, and new culture-specific test. |
| TestWithMoreThan6Params.generated.txt | Baseline updated to wrap the interpolated string with string.Create(InvariantCulture, ...). |
| LoggerMessageGenerator.Roslyn4.0.cs LoggerMessageGenerator.Roslyn3.11.cs |
Pass Compilation into Emitter instead of using the parameterless constructor. |
| LoggerMessageGenerator.Emitter.cs | Extended Emitter to detect a string.Create overload and conditionally wrap generated interpolations. |
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs:15
- The class declaration syntax with a parameter list is invalid in C#. You should declare a private readonly field for
Compilation, define a constructorpublic Emitter(Compilation compilation)inside the class, and assign the parameter to the field so the code compiles.
internal sealed class Emitter(Compilation compilation)
|
Tagging subscribers to this area: @dotnet/area-extensions-logging |
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs
Show resolved
Hide resolved
...tions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM, Thanks!
Do we need to track fix the same in the Extensions/R9 generator?
CC @joperezr @geeknoid @evgenyfedorov2
update
Looks extensions generator already support it? https://github.com/dotnet/extensions/blob/3d2ddda238190cd9c5730227d070a03554c151b5/src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs#L119
|
@tarekgh I just took a quick peek at the dotnet/extensions code and I think it should be fine. It would be worth running the test case from the bug report against that generator just to make sure though. |
|
I have tried the repo from the issue on the extensions source gen, and it works fine, producing the output using the invariant culture as expected: |
|
I forgot we have two of these 😦 |
Fixes #116975