Skip to content
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

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Mar 18, 2024

ILogger.Parameters is documented as allowing a null value. This change updates the annotation accordingly.

CPS explicitly returns null from its implementation of this interface.

`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.
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@YuliiaKovalova YuliiaKovalova merged commit 2372c72 into main Mar 27, 2024
9 checks passed
@YuliiaKovalova YuliiaKovalova deleted the dev/drnoakes/null-annotate-ilogger branch March 27, 2024 13:42
rainersigwald added a commit to dotnet/sdk that referenced this pull request May 1, 2024
rainersigwald added a commit to rainersigwald/sdk that referenced this pull request May 1, 2024
@rainersigwald
Copy link
Member

So this was a breaking change that required reaction in SDK. I think we might need to revert it? @dotnet/kitten

@drewnoakes
Copy link
Member Author

How was this breaking? I can't see anything in the cross referenced PR.

@rainersigwald
Copy link
Member

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 ? on Parameters.

@ladipro
Copy link
Member

ladipro commented May 2, 2024

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.

@rainersigwald
Copy link
Member

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.

v-wuzhai pushed a commit to dotnet/sdk that referenced this pull request May 6, 2024
Forgind pushed a commit to Forgind/sdk that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants