Enable nullable annotations for M.E.L.Abstractions #43892

maryamariyan · 2020-10-27T15:17:08Z

cc: @buyaa-n @safern

Related to #43605

ghost · 2020-10-27T15:17:11Z

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LoggerExternalScopeProvider.cs

src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogEntry.cs

maryamariyan · 2020-10-27T15:32:40Z

src/libraries/Microsoft.Extensions.Logging.Abstractions/src/FormattedLogValues.cs

Note for reviewer: _values is not null due to what Count returns on line 76

src/libraries/Common/src/Extensions/TypeNameHelper/TypeNameHelper.cs

src/libraries/Common/src/Extensions/Logging/NullExternalScopeProvider.cs

...s/Microsoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs

src/libraries/Common/src/Extensions/TypeNameHelper/TypeNameHelper.cs

krwq · 2020-10-29T16:32:41Z

...s/Microsoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs

please make sure owner of this library double checks the annotations of the ref

...s/Microsoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs

stephentoub · 2020-10-29T16:37:12Z

...crosoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj

Did you intend to check in these changes?

yes this was intentional otherwise wouldn't have been able to add NetCoreAppCurrent for the project I think.

cc: @safern

The reason behind adding NetCoreAppCurrent for these projects was to have CI protection for the nullable annotations to actually be correct and not broken on by upcoming changes but to actually not ship that asset. However the only caveat that this brings is that by adding this I'm not sure if people can just clone the repo and build this project without pre building anything else which was an effort @ViktorHofer did in the past.

yes this was intentional otherwise wouldn't have been able to add NetCoreAppCurrent for the project I think.

This property is for convenience only, in case you don't want to list all the individual assembly references. In this case I think it's fine as we don't ship the `$(NetCoreAppCurrent)' asset.

However the only caveat that this brings is that by adding this I'm not sure if people can just clone the repo and build this project without pre building anything

Unfortunately with our current infra that's true but I don't think we should see that as a blocker. I would like to improve our infra so that $(NetCoreAppCurrent) builds via dependencies without an up-front build which would also enable VS builds for .NETCoreApp without up-front steps.

stephentoub

Overall LGTM.

src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogEntry.cs

krwq

LGTM with some questions

to allow annotations on relevant projects, not yet enabled

maryamariyan · 2020-11-05T13:48:58Z

@stephentoub @krwq @tarekgh looked at all the comments and made all the changes I was planning to do.

I posed this question in #43892 (comment) too.

Would it be reasonable to restrict formatter argument in ILogger.Write(.., formatter) to be not nullable and make TraceSourceLogger consistent with the others through this change? (refer to API diff for making formatter not nullable)

let me know what you think.

maryamariyan · 2020-11-09T18:58:02Z

Updates on PR since it was last reviewed:

Addressed PR feedback
LoggerExtensions.cs: params object?[] -> params object?[]
Last commit makes formatter non-nullable

maryamariyan · 2020-11-12T22:20:16Z

@stephentoub @krwq could you please re-review?

maryamariyan · 2020-11-12T22:23:15Z

perf legs usually don't run on PRs normally wonder why they showed up here.

cc: @ericstj

@safern is this something to be concerned about?

ViktorHofer · 2020-11-12T22:46:45Z

cc @DrewScoggins

...Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj

…ons-null

…ons-null Conflicts: src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj

ghost · 2020-11-17T14:05:25Z

Hello @maryamariyan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

stephentoub · 2021-05-24T13:17:09Z

@maryamariyan, doesn't the ref csproj need <Nullable>enable</Nullable> as well?

maryamariyan added the area-Extensions-Logging label Oct 27, 2020

maryamariyan self-assigned this Oct 27, 2020

maryamariyan commented Oct 27, 2020

View reviewed changes

maryamariyan force-pushed the logging-abstractions-null branch from edae4e5 to 8df11e4 Compare October 27, 2020 19:58

maryamariyan requested review from krwq, stephentoub and tarekgh October 27, 2020 20:52

krwq reviewed Oct 29, 2020

View reviewed changes

src/libraries/Common/src/Extensions/Logging/NullExternalScopeProvider.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 29, 2020

View reviewed changes

...s/Microsoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 29, 2020

View reviewed changes

...s/Microsoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 29, 2020

View reviewed changes

...s/Microsoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs Outdated Show resolved Hide resolved

krwq reviewed Oct 29, 2020

View reviewed changes

src/libraries/Common/src/Extensions/TypeNameHelper/TypeNameHelper.cs Outdated Show resolved Hide resolved

krwq reviewed Oct 29, 2020

View reviewed changes

...s/Microsoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 29, 2020

View reviewed changes

krwq reviewed Oct 29, 2020

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogEntry.cs Outdated Show resolved Hide resolved

krwq approved these changes Oct 29, 2020

View reviewed changes

maryamariyan force-pushed the logging-abstractions-null branch 3 times, most recently from 70e6b56 to c3e738b Compare November 4, 2020 18:09

maryamariyan added 5 commits November 4, 2020 18:46

Enable nullable annotations for M.E.L.Abstractions

2bc1203

Corrections from review

fdca282

LoggerExtensions.cs: params object?[] -> params object?[]

dbe0de7

using Nullable Annotations,

5c4193e

to allow annotations on relevant projects, not yet enabled

formatter not null

0ad32e6

maryamariyan force-pushed the logging-abstractions-null branch from c3e738b to 0ad32e6 Compare November 5, 2020 13:46

krwq reviewed Nov 16, 2020

View reviewed changes

...Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj Show resolved Hide resolved

maryamariyan added 2 commits November 17, 2020 05:58

Merge remote-tracking branch 'upstream/master' into logging-abstracti…

3db4fdb

…ons-null

Merge remote-tracking branch 'upstream/master' into logging-abstracti…

d2b969f

…ons-null Conflicts: src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj

maryamariyan added the auto-merge label Nov 17, 2020

ghost merged commit fa06656 into dotnet:master Nov 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 18, 2020

This pull request was closed.

Enable nullable annotations for M.E.L.Abstractions #43892

Enable nullable annotations for M.E.L.Abstractions #43892

Uh oh!

Conversation

maryamariyan commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 27, 2020

Uh oh!

Uh oh!

Uh oh!

maryamariyan Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krwq Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyan Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krwq left a comment

Choose a reason for hiding this comment

Uh oh!

maryamariyan commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryamariyan commented Nov 9, 2020

Uh oh!

maryamariyan commented Nov 12, 2020

Uh oh!

maryamariyan commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Nov 12, 2020

Uh oh!

Uh oh!

ghost commented Nov 17, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

stephentoub commented May 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maryamariyan commented Oct 27, 2020 •

edited

Loading

maryamariyan Nov 4, 2020 •

edited

Loading

ViktorHofer Nov 11, 2020 •

edited

Loading

maryamariyan commented Nov 5, 2020 •

edited

Loading

maryamariyan commented Nov 12, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.