-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Limit language version on Logger and Options source gens #90654
Limit language version on Logger and Options source gens #90654
Conversation
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue DetailsFixes #83393 These source generators generate code using nullable feature which not supported before C# 8. The change here is require at least C# 8 for source generation. Candidate to port to rc1
|
@eiriktsarpalis can you please help review this as I am planning to post to rc1? Thanks! |
Is this only about use of |
Yes. Mostly about |
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/5885027700 |
<value>C# language version not supported by the source generator.</value> | ||
</data> | ||
<data name="LoggingUnsupportedLanguageVersionMessageFormat" xml:space="preserve"> | ||
<value>The Logging source generator is not available in C# '{0}'. Please use language version '{1}' or greater.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be removing quotes from both places rather than adding them. We control the version enum that is being passed in both cases, so there's no need to guard with quotes.
I'll submit a new PR changing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not seeing having the quotes is bad to show in the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quotes are typically used to delimit inputs that might contain arbitrary values, including whitespace. We control the inputs passed to the diagnostic and it's kind of weird to say C# '8'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a PR but somehow didn't add you as a reviewer:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quotes are typically used to delimit inputs that might contain arbitrary values, including whitespace. We control the inputs passed to the diagnostic and it's kind of weird to say C# '8'.
Not necessary, sometimes the quotes can be used to emphasize the info inside the quote. Anyway, I don't mind either way. I'll take a look at the PR.
Fixes #83393
These source generators generate code using nullable feature which not supported before C# 8. The change here is require at least C# 8 for source generation.
Candidate to port to rc1