-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 ConfigurationManager #55338
Add ConfigurationManager #55338
Conversation
Note regarding the 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. |
Tagging subscribers to this area: @maryamariyan, @safern Issue DetailsThis adds a type already used by ASP.NET Core's new WebApplcation and WebApplicationBuilder that allows reading from configuration (e.g. New APInamespace Microsoft.Extensions.Configuration
{
+ public sealed class Config : IConfigurationRoot, IConfigurationBuilder, IDisposable
+ {
+ public Configuration();
+ public string? this[string key] { get; set; }
+ public IConfigurationSection GetSection(string key);
+ public void Dispose();
+ } The members that are required to implement IConfigurationBuilder will be implemented explicitly, so members like Usage Example
using var config = new Config();
config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");
if (config["FileConfig"] == "enabled")
{
config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
}
string myValueFromJson = config["JsonConfigValue"];
// ... We have docs where we demonstrate manually building an IConfigurationBuilder, reading from the built IConfiguration, and then throwing that away to add a new config source. This is an alternative to that. Fixes #51770
|
@pakrym @davidfowl Does this look good to you? |
Did we get a consensus on the name yet? As it was stated on the issue I don't think |
@safern We're clearing up the naming but can you review the PR? |
src/libraries/Microsoft.Extensions.Configuration/tests/ConfigTests.cs
Outdated
Show resolved
Hide resolved
var memVal2 = config["Mem2:KeyInMem2"]; | ||
var memVal3 = config["MEM3:KEYINMEM3"]; | ||
|
||
// Assert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This comment doesn't seem that useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally agree about the uselessness of // Arrange // Act // Assert
comments, but this was copied from ConfigurationTest. If we change it, I'd rather do it in one pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I don't mind if we do that cleanup in a separate PR.
src/libraries/Microsoft.Extensions.Configuration/tests/ConfigTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Configuration/tests/ConfigTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds a type already used by ASP.NET Core's new WebApplcation and WebApplicationBuilder that allows reading from configuration (e.g.
appsettings.json
andDOTNET_
/ASPNETCORE_
environment variables) while still being able to add new configuration sources without explicitly rebuilding config. Every time a source is added via theIConfigurationBuilder
interface, theIConfiguration
updates automatically and immediately.New API
The members that are required to implement IConfigurationBuilder will be implemented explicitly, so members like
IList<IConfigurationSource> IConfigurationBuilder.Sources
don't pollute intellisense. Extension methods are generally used to add configuration sources.Usage Example
WebApplicationBuilder
essentially already exposes this type as a public property. The problem it is trying to solve is being able to read config fromappsettings.json
andDOTNET_
/ASPNETCORE_
while configuring the host'sIServiceCollection
while at the same time being able to add new configuration sources or even change the content root without having to introduce additional build stages.We have docs where we demonstrate manually building an IConfigurationBuilder, reading from the built IConfiguration, and then throwing that away to add a new config source. This is an alternative to that.
Fixes #51770