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.

Sign up for GitHub

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

Jump to bottom

[Group 4] Enable nullable annotations for Microsoft.Extensions.Configuration.Binder #57418

Merged
merged 19 commits into from
Nov 18, 2021
Merged

[Group 4] Enable nullable annotations for Microsoft.Extensions.Configuration.Binder #57418

merged 19 commits into from
Nov 18, 2021

Conversation

maxkoshevoi
Copy link
Contributor

@maxkoshevoi maxkoshevoi commented Aug 14, 2021 •
edited
Loading

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 14, 2021
@dotnet-issue-labeler
Copy link

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Configuration new-api-needs-documentation and removed community-contribution Indicates that the PR has been added by a community member labels Aug 14, 2021
@ghost
Copy link

ghost commented Aug 14, 2021

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

Issue Details

Related to #43605, #54012

Notes:

Author: maxkoshevoi
Assignees: -
Labels:

new-api-needs-documentation, area-Extensions-Configuration
Milestone: -

@maxkoshevoi maxkoshevoi changed the title Enable nullable annotations for Microsoft.Extensions.Configuration.Binder [Group 4] Enable nullable annotations for Microsoft.Extensions.Configuration.Binder Aug 15, 2021
@maryamariyan maryamariyan added this to the 7.0.0 milestone Aug 16, 2021
@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Closed
@maxkoshevoi
Merge branch 'main' into mk/43605-Configuration-Binder
9004a7c
@maxkoshevoi
DisableImplicitAssemblyReferences
de8bea7
@maxkoshevoi
Nullablity fix
e41470b
@maxkoshevoi
Merge branch 'main' into mk/43605-Configuration-Binder
809f71d 
# Conflicts:
#	src/libraries/Microsoft.Extensions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.csproj
#	src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/Microsoft.Extensions.Configuration.Abstractions.csproj
#	src/libraries/Microsoft.Extensions.Configuration.Binder/ref/Microsoft.Extensions.Configuration.Binder.csproj
#	src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj
@maxkoshevoi
Merge branch 'main' into mk/43605-Configuration-Binder
4f9e6d7 
# Conflicts:
#	src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
@maxkoshevoi
configureOptions are non-nullable
5dac613
@maxkoshevoi
Revert "configureOptions are non-nullable"
d5e359f 
This reverts commit 5dac613.
@eerhardt
Copy link
Member

eerhardt commented Oct 4, 2021 •
edited
Loading

@maxkoshevoi - let's concentrate on getting the Group 1, 2 and 3 libraries completed. Since it takes a lot of effort to review these PRs, and they have been sitting for a while, what do you think about closing the "Group 4" PRs for now? And we can open them again when the lower level libraries are complete?

@maxkoshevoi
Copy link
Contributor Author

@eerhardt What's the harm in keeping them open? They won't be reviewed until parent PRs are merged anyway (#57401 in this case). Or do you just want to do it for some internal metric?
I can convert Group 4 PRs into drafts. That also should do the trick.

@eerhardt
Copy link
Member

eerhardt commented Oct 4, 2021

What's the harm in keeping them open? Or do you just want to do it for some internal metric?

We do have an effort of trying to keep on top of our open PRs. As you can see, we currently have over 220 open PRs, some of them have been open for months with no activity.

The harm is someone coming to the PR and trying to review it, but it isn't ready to be reviewed. It also affects managing the PRs ("these ones can be reviewed" vs. "these ones are waiting for something").

@eerhardt
Copy link
Member

eerhardt commented Oct 4, 2021

I can convert Group 4 PRs into drafts.

I think that would be great for now. Thanks!

@maxkoshevoi maxkoshevoi marked this pull request as draft October 4, 2021 19:03
@maxkoshevoi maxkoshevoi marked this pull request as ready for review October 6, 2021 21:42
@runfoapp runfoapp bot mentioned this pull request Oct 7, 2021
Closed
@maxkoshevoi maxkoshevoi mentioned this pull request Oct 13, 2021
Merged
@maxkoshevoi
Merge branch 'main' into mk/43605-Configuration-Binder
ec07505 
# Conflicts:
#	src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
eerhardt
eerhardt reviewed Nov 11, 2021
View reviewed changes
...crosoft.Extensions.Configuration.Binder/ref/Microsoft.Extensions.Configuration.Binder.csproj Outdated Show resolved Hide resolved
...crosoft.Extensions.Configuration.Binder/ref/Microsoft.Extensions.Configuration.Binder.csproj Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
type: itemType,
instance: null,
config: section,
options: options);
if (item != null)
{
addMethod.Invoke(collection, new[] { item });
addMethod?.Invoke(collection, new[] { item });
Copy link
Member

Choose a reason for hiding this comment

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

If there is no Add method, this will now silently no longer do anything. Is that what we want to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't seem to do anything before (there's an empty catch on the line bellow).

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved
eerhardt
eerhardt approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution here, @maxkoshevoi.

I pushed a change that should allow CI to pass successfully. Will merge when the CI is green.

@eerhardt eerhardt merged commit c0dabb5 into dotnet:main Nov 18, 2021
@eerhardt eerhardt mentioned this pull request Nov 18, 2021
Merged
@maxkoshevoi maxkoshevoi deleted the mk/43605-Configuration-Binder branch November 18, 2021 21:42
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
area-Extensions-Configuration new-api-needs-documentation
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@maxkoshevoi @eerhardt @maryamariyan