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 3] Enable nullable annotations for Microsoft.Extensions.Configuration #57414

Merged
merged 24 commits into from
Oct 13, 2021
Merged

[Group 3] Enable nullable annotations for Microsoft.Extensions.Configuration #57414

merged 24 commits into from
Oct 13, 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.

@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, community-contribution
Milestone: -

@maxkoshevoi maxkoshevoi mentioned this pull request Aug 14, 2021
Closed
This was referenced Aug 15, 2021
Merged
Merged
Merged
@maxkoshevoi maxkoshevoi changed the title Enable nullable annotations for Microsoft.Extensions.Configuration [Group 3] Enable nullable annotations for Microsoft.Extensions.Configuration 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 and others added 8 commits August 27, 2021 00:48
@maxkoshevoi
Merge branch 'main' into mk/43605-Configuration
051b15b
@maxkoshevoi
DisableImplicitAssemblyReferences
5a7a6d3
@maxkoshevoi
Merge branch 'main' into mk/43605-Configuration
9bde07c 
# 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/ref/Microsoft.Extensions.Configuration.csproj
#	src/libraries/Microsoft.Extensions.Configuration/src/Microsoft.Extensions.Configuration.csproj
@maxkoshevoi
TryGet can return null
4945112
@maxkoshevoi
Merge branch 'main' into mk/43605-Configuration
330dc3a
@maxkoshevoi
new()
f320ed0
@eerhardt
Merge remote-tracking branch 'upstream/main' into mk/43605-Configuration
7cbd323
@eerhardt
Fix merge error
b9a0a93
This was referenced Oct 7, 2021
Closed
Closed
safern
safern reviewed Oct 8, 2021
View reviewed changes
src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationProvider.cs
{
var results = new List<string>();

if (parentPath is null)
{
foreach (KeyValuePair<string, string> kv in Data)
foreach (KeyValuePair<string, string?> kv in Data)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: not related to this PR but it seems like we could just iterate over Data.Keys rather than KVP? We are just using the keys on these two for loops.

Copy link
Contributor Author

@maxkoshevoi maxkoshevoi Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@eerhardt is against non-nullable-related changes in nullable PRs. I've already reverted some of them a45a821.

eerhardt
eerhardt approved these changes Oct 12, 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.

I think this looks good. @safern - any additional feedback?

Thanks for the contribution here, @maxkoshevoi.

safern
safern approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@safern
Copy link
Member

safern commented Oct 13, 2021

Merging as the Fuzzlyn pipeline is not ready yet and not supposed to run on PRs. cc: @kunalspathak

@safern safern merged commit 6033ef2 into dotnet:main Oct 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2021
@maxkoshevoi maxkoshevoi deleted the mk/43605-Configuration branch January 19, 2022 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern approved these changes

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
7.0.0
Development

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