Skip to content

[Group 3] Enable nullable annotations for Microsoft.Extensions.Logging - #65262

Merged
eerhardt merged 29 commits into
dotnet:mainfrom
maxkoshevoi:mk/43605-logging
Mar 18, 2022
Merged

[Group 3] Enable nullable annotations for Microsoft.Extensions.Logging#65262
eerhardt merged 29 commits into
dotnet:mainfrom
maxkoshevoi:mk/43605-logging

Conversation

@maxkoshevoi

Copy link
Copy Markdown
Contributor

Related to #43605

Questions:

@ghostghost added area-Extensions-Logging new-api-needs-documentation community-contribution Indicates that the PR has been added by a community member labels Feb 12, 2022
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #43605

Questions:

Author:maxkoshevoi
Assignees:-
Labels:

new-api-needs-documentation, area-Extensions-Logging, community-contribution

Milestone:-

@maxkoshevoi

Copy link
Copy Markdown
ContributorAuthor

Have no idea, why CI fails. ref signatures looks the same as src, and dotnet msbuild /t:GenerateReferenceAssemblySource doesn't change anything.

@maryamariyan

maryamariyan commented Mar 1, 2022

Copy link
Copy Markdown
Contributor

Have no idea, why CI fails. ref signatures looks the same as src, and dotnet msbuild /t:GenerateReferenceAssemblySource doesn't change anything.

looking

@maryamariyan

maryamariyan commented Mar 2, 2022

Copy link
Copy Markdown
Contributor

I noticed that running

> dotnet build src\libraries\Microsoft.Extensions.Logging\src\ /t:GenerateReferenceAssemblySource -c Release

auto generates the small diff in 23dc4b5 .

Would you like to push this commit onto this PR to see if retriggering CI this way would fix the CI failures?

Comment threadsrc/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Logging/src/LoggerInformation.cs Outdated

@maryamariyanmaryamariyan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aside the question in https://github.com/dotnet/runtime/pull/65262/files#r817973746 the rest looks fine to me.

@maryamariyanmaryamariyan self-assigned this Mar 3, 2022
@maxkoshevoi

Copy link
Copy Markdown
ContributorAuthor

@maryamariyan Hi, I'm in Ukraine (currently relatively safe in Lviv), so cannot be as active. I'll try to address the comments when I can, but if I don't respond for tool long or disappear altogether, feel free to commit to my branches.

@joperezr

Copy link
Copy Markdown
Member

You need to invoke Pack in order for package validation to run, and for it to emit the baseline file. Try doing: \src\libraries\Microsoft.Extensions.Logging\src> dotnet build /p:GenerateCompatibilitySuppressionFile=true /p:BuildAllConfigurations=true (passing in the BuildAllConfigurations=true option triggers GeneratePackageOnBuild to be set to true which builds the package)

Comment threadsrc/libraries/Microsoft.Extensions.Logging/src/LoggerInformation.cs Outdated

@maryamariyanmaryamariyan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM aside from the ScopeLogger portion in #65262 (comment)

Comment threadsrc/libraries/Microsoft.Extensions.Logging/src/LoggerInformation.cs Outdated

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great. Thanks again for all the work here, @maxkoshevoi!

@eerhardt
eerhardt merged commit 2306813 into dotnet:mainMar 18, 2022
@maxkoshevoi
maxkoshevoi deleted the mk/43605-logging branch March 18, 2022 15:20
@SteveDunn

Copy link
Copy Markdown
Contributor

@maryamariyan Hi, I'm in Ukraine (currently relatively safe in Lviv), so cannot be as active. I'll try to address the comments when I can, but if I don't respond for tool long or disappear altogether, feel free to commit to my branches.

Stay safe my friend.

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…ng` (dotnet#65262)
* First pass
* Second pass
* Pass three
* Missed few things
* ref
* Update ProviderAliasUtilities.cs
* Update ActivityTrackingOptions.cs
* Update Microsoft.Extensions.Logging.cs
* GetRequiredService
* Update Logger.Scope
* Create CompatibilitySuppressions.xml
* Add comment to CompatibilitySuppressions.xml
Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
* Delete CompatibilitySuppressions.xml
* AsyncLocal<Scope?> _currentScope
* Add assert
Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Apr 26, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Extensions-Loggingcommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@maxkoshevoi@maryamariyan@joperezr@SteveDunn@eerhardt