-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Null annotate ILogger.cs #9876
Null annotate ILogger.cs #9876
Conversation
`ILogger.Parameters` is documented as allowing a `null` value. This change updates the annotation accordingly. NOTE this change has been made in the browser without building, so it's possible that CI highlights other usages that need fixing.
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.
Thank you!
So this was a breaking change that required reaction in SDK. I think we might need to revert it? @dotnet/kitten |
How was this breaking? I can't see anything in the cross referenced PR. |
It can break third-party loggers in the same way it broke the two null-annotated loggers in the MSBuild codebase, by requiring them to add a |
I think I would consider this an acceptable source breaking change. Core libraries do this as well, for example https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/nullable-ref-type-annotation-changes. |
They do, with explicit breaking change docs, once a year. I agree it's a good change, just not sure what our pain/benefit policy is there. |
ILogger.Parameters
is documented as allowing anull
value. This change updates the annotation accordingly.CPS explicitly returns null from its implementation of this interface.