Skip to content

Feature/get debug view value processing - #60391

Merged
safern merged 21 commits into
dotnet:mainfrom
oskrabanek:feature/get-debug-view-value-processing
Dec 1, 2021
Merged

Feature/get debug view value processing#60391
safern merged 21 commits into
dotnet:mainfrom
oskrabanek:feature/get-debug-view-value-processing

Conversation

@oskrabanek

@oskrabanekoskrabanek commented Oct 14, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Implementation of API proposal #60065 based on the changes proposed on the review
@safern

@ghostghost added community-contribution Indicates that the PR has been added by a community member area-Extensions-Configuration labels Oct 14, 2021
@ghost

ghost commented Oct 14, 2021

Copy link
Copy Markdown

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

Issue Details

Implementation of API proposal #60065

Author:Andree643
Assignees:-
Labels:

area-Extensions-Configuration, community-contribution

Milestone:-

safern
safern reviewed Oct 18, 2021
View reviewed changes
Comment thread...braries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationDebugViewContext.cs Outdated
/// <summary>
/// Value of the current item
/// </summary>
public string Value { get; }

@safernsafernOct 18, 2021

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.

Should Value be nullable?

@oskrabanekoskrabanekOct 18, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

As mentioned in the issue comment value is non-nullable by definition of the IConfigurationProvider.

@safernsafernOct 18, 2021

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.

It seems like it is conflicting with valueAndProvider.Value as that seems to be nullable and the build is failing because of that.

safern
safern reviewed Oct 18, 2021
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationRootExtensions.cs Outdated
}

return (null, null);
return ("", null);

@safernsafernOct 18, 2021

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.

Why are we changing this behavior?

@safern

safern commented Oct 18, 2021

Copy link
Copy Markdown
Member

Could you also update: https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.cs to add the new APIs?

Here are instructions on how to do that: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md

@runfoapprunfoappBot mentioned this pull request Oct 27, 2021
Closed
safern
safern reviewed Oct 27, 2021
View reviewed changes

@safernsafern 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.

@Andree643 looks good overall! Could you also please add some tests for the new API?

@oskrabanek

oskrabanek commented Oct 28, 2021

Copy link
Copy Markdown
ContributorAuthor

@Andree643 looks good overall! Could you also please add some tests for the new API?

Sure, will do.

Ondřej Škrabánek@safern
Update src/libraries/Microsoft.Extensions.Configuration.Abstractions/…
73f8850
…src/ConfigurationRootExtensions.cs
Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com>
@eerhardt

eerhardt commented Nov 18, 2021

Copy link
Copy Markdown
Member

Hi @Andree643 - do you have an idea when the tests for the new API will be added? Let us know if you are interested in pushing this PR forward.

@oskrabanek

oskrabanek commented Nov 19, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Hi @Andree643 - do you have an idea when the tests for the new API will be added? Let us know if you are interested in pushing this PR forward.

Hi @eerhardt. I'll do my best to make it next week. I have some issues with building the solution and trying to resolve it.

eerhardt
eerhardt reviewed Nov 30, 2021
View reviewed changes

@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.

Looks pretty good. I just had some nits on some doc comments. Once those are addressed, I believe this can be merged.

Comment thread...braries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationDebugViewContext.cs Outdated
Comment thread...braries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationDebugViewContext.cs Outdated
Comment thread...braries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationDebugViewContext.cs Outdated
Comment thread...braries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationDebugViewContext.cs Outdated
Comment thread...libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationRootExtensions.cs Outdated
Comment thread...braries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationDebugViewContext.cs Outdated
@eerhardteerhardt self-assigned this Nov 30, 2021
Ondřej Škrabánek@eerhardt
Update src/libraries/Microsoft.Extensions.Configuration.Abstractions/…
8801896
…src/ConfigurationDebugViewContext.cs
Docs update
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Ondřej Škrabánekand others added 5 commits December 1, 2021 07:30
Ondřej Škrabánek@eerhardt
Update src/libraries/Microsoft.Extensions.Configuration.Abstractions/…
a8b4341
…src/ConfigurationDebugViewContext.cs
Docs updateDocs update
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Ondřej Škrabánek@eerhardt
Update src/libraries/Microsoft.Extensions.Configuration.Abstractions/…
0dbb60a
…src/ConfigurationDebugViewContext.cs
Docs update
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Ondřej Škrabánek@eerhardt
Update src/libraries/Microsoft.Extensions.Configuration.Abstractions/…
d2180b8
…src/ConfigurationDebugViewContext.cs
Docs update
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Ondřej Škrabánek@eerhardt
Update src/libraries/Microsoft.Extensions.Configuration.Abstractions/…
acaa1a0
…src/ConfigurationRootExtensions.cs
Docs update
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Ondřej Škrabánek@eerhardt
Update src/libraries/Microsoft.Extensions.Configuration.Abstractions/…
79c2868
…src/ConfigurationDebugViewContext.cs
Docs update
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@safern
safern merged commit 5528e84 into dotnet:mainDec 1, 2021
@oskrabanek
oskrabanek deleted the feature/get-debug-view-value-processing branch December 3, 2021 08:42
@ghostghost locked as resolved and limited conversation to collaborators Jan 2, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eerhardteerhardteerhardt left review comments
@safernsafernsafern left review comments

Assignees

@eerhardteerhardt

Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@oskrabanek@safern@eerhardt