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.

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

Add a method GetRequiredSection on Configuration #40976

Merged
merged 10 commits into from
Aug 21, 2020

Conversation

michelcedric
Copy link
Contributor

@michelcedric michelcedric commented Aug 18, 2020

Add a method GetRequiredSection as a GetRequiredServices, it raised an exception.
In this casse if the section was not found it raised an InvalidOperationException

FIxes: #40978

@Dotnet-GitSync-Bot
Copy link
Collaborator

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-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Aug 18, 2020
@michelcedric
Copy link
Contributor Author

michelcedric commented Aug 18, 2020

Hi @jkotas, @maryamariyan
I think you can add the area-Extensions-Configuration?
I made my pull request on the correct branch because I made a mistake the first time. #40137

I also created an issue : #40978

@michelcedric
Copy link
Contributor Author

Hi @ericstj
I add you on this PR, if it's correct you are the Lead for area-Extensions-Configuration
And also @safern

@ericstj ericstj added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 18, 2020
@ericstj
Copy link
Member

ericstj commented Aug 18, 2020

Hi @michelcedric, thank you for the contribution. Let's wait for API approval before reviewing this. I've marked it as NO MERGE for now.

@ericstj ericstj added area-Extensions-Configuration and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 18, 2020
@ghost
Copy link

ghost commented Aug 18, 2020

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

@michelcedric
Copy link
Contributor Author

@ericstj
The linked issue is on api-ready-for-review label.
#40978
I hope this one can be approve.

@ericstj
Copy link
Member

ericstj commented Aug 18, 2020

We don't review PRs that are for unapproved API as the API may significantly change invalidating the review. For example this PR adds a method to an interface which breaks all implementers of that interface, this is a significant breaking change that will likely not be approved as-is.

@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Aug 18, 2020
@michelcedric
Copy link
Contributor Author

@ericstj
Thanks for your answer. I understant your process.
But I'm not see a breaking change. I adapt the implementers of this interface. I maybe forget something? Because for the moment, i not see a breaking change? You maybe talk if someone make a proper implementation of IConfiguration?

@michelcedric
Copy link
Contributor Author

@ericstj
@terrajobst made this proposal.
#40978 (comment)
I think it's maybe better and not introduce breaking change, if you prefer, I can adapt the pull request?

@ericstj
Copy link
Member

ericstj commented Aug 18, 2020

I adapt the implementers of this interface. I maybe forget something? Because for the moment, i not see a breaking change? You maybe talk if someone make a proper implementation of IConfiguration?

It's a public interface. Implementers exist outside of this repository. If some 3rd party implements IConfiguration and they try to use their library after this fix, their code would fail to bind with a TypeLoadException. You can mitigate that by giving your interface a default implementation, but this is all conversation for API review (which I think @terrajobst is pointing out).

@ericstj ericstj removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels Aug 18, 2020
@ericstj ericstj requested a review from maryamariyan August 18, 2020 19:13
@eerhardt
Copy link
Member

//------------------------------------------------------------------------------

We don't use a Designer.cs file in dotnet/runtime. An SR class is generated by the build. You are already using that in your code, but this file should be deleted (and removed from the .csproj).


Refers to: src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/Resources/Strings.Designer.cs:1 in 494eb5e. [](commit_id = 494eb5e, deletion_comment = False)

Copy link
Contributor Author

@michelcedric michelcedric left a comment

Choose a reason for hiding this comment

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

I fix your remarks sorry it's my first contribution

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.

sorry it's my first contribution

Welcome, I'm happy you are here contributing 😄. There's no need to apologize.

This looks good to me. Thanks for getting this proposed and implemented.

…src/ConfigurationExtensions.cs

Co-authored-by: Eric Erhardt <[email protected]>
Copy link
Member

@maryamariyan maryamariyan left a comment

Choose a reason for hiding this comment

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

Thanks @michelcedric

@safern
Copy link
Member

safern commented Aug 21, 2020

@maryamariyan @eerhardt is this API approved? Can we merge?

@maryamariyan
Copy link
Member

yes I think the outstanding things were the NOMERGE label that @ericstj added earlier and the CI wasnt green yet. Otherwise it is good to go.

@safern
Copy link
Member

safern commented Aug 21, 2020

Thanks, @maryamariyan -- merging since master is already 6.0.0.

@safern safern merged commit 89292fe into dotnet:master Aug 21, 2020
@michelcedric
Copy link
Contributor Author

Hi @safern , @maryamariyan , @eerhardt
Thanks you to merged in master Branch : but what represent the Master branch.
You talked about 6.0.0, it's signify, this feature should be present in .net 6 in more than 1 year?
Or in .Net 5 or minor version?
For the moment I'm not understand your git branching..
Do you have a documentation?

@safern
Copy link
Member

safern commented Aug 21, 2020

release/5.0 is the branch for .NET 5 and .NET 5.1 will branch of off it once .NET5.0 is released into release/5.1.

Master is the work that is targeting for .NET 6 next year.

@mmitche is there any documentation on the branching model we have?

@michelcedric
Copy link
Contributor Author

@safern
Thanks you for your time and answer, in this case to find my little feature in release, I waiting .net 6. It's so long ;) :)

@safern
Copy link
Member

safern commented Aug 21, 2020

You can always use preview builds of the package 😄 -- we release daily builds of packages. Maybe later today or tomorrow we will have a build of this package with your figure.

@mmitche
Copy link
Member

mmitche commented Aug 21, 2020

release/5.0 is the branch for .NET 5 and .NET 5.1 will branch of off it once .NET5.0 is released into release/5.1.

Master is the work that is targeting for .NET 6 next year.

@mmitche is there any documentation on the branching model we have?

There is not a 5.1 AFAIK. So master is .NET 6 (next fall), and release/5.0 is the ongoing servicing for .NET 5. I don't think we have any docs on branching structure other than the constantly shifting record that is in Maestro.

@eerhardt
Copy link
Member

Note here is our roadmap which details planned releases: https://github.com/dotnet/core/blob/master/roadmap.md

@safern
Copy link
Member

safern commented Aug 21, 2020

There is not a 5.1 AFAIK. So master is .NET 6

Ah right, .NET 6.0 is the next LTS.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add GetRequiredSection on Configuration (raise exception if missing section)
8 participants