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

Support immutable types with configuration binding #43662

Closed
Tracked by #47191
davidfowl opened this issue Oct 20, 2020 · 17 comments · Fixed by #67258
Closed
Tracked by #47191

Support immutable types with configuration binding #43662

davidfowl opened this issue Oct 20, 2020 · 17 comments · Fixed by #67258

Comments

@davidfowl
Copy link
Member

Similar to #43359 but specifically for configuration binding. Support binding from IConfiguration to say a record or any immutable .NET object. This has been solved in JSON (it's basically deserialization) and we can use any lessons learned there to implement a solution in configuration.

public record Settings(string Color, int Length);

public void ConfigureServices(IServiceCollection services)
{
    services.Configure<Settings>(Configuration.GetSection("MySettings"));
}
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-Configuration untriaged New issue has not been triaged by the area owner labels Oct 20, 2020
@ghost
Copy link

ghost commented Oct 20, 2020

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

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Oct 22, 2020
@maryamariyan maryamariyan added this to Uncommitted in ML, Extensions, Globalization, etc, POD. via automation Oct 22, 2020
@maryamariyan maryamariyan added this to the 6.0.0 milestone Oct 22, 2020
@ghost
Copy link

ghost commented Oct 28, 2020

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

@ghost
Copy link

ghost commented Oct 28, 2020

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

@eerhardt
Copy link
Member

6.0 is now feature complete. Moving to 7.0.

@maryamariyan maryamariyan moved this from Untriaged to 7.0.0 in ML, Extensions, Globalization, etc, POD. Aug 4, 2021
@aradalvand
Copy link

Disappointing that this was not implemented in 6.0 to be honest.

@SteveDunn
Copy link
Contributor

@maryamariyan - I'm happy to look at this one

@maryamariyan
Copy link
Member

maryamariyan commented Mar 23, 2022

@maryamariyan - I'm happy to look at this one

@SteveDunn sure looking forward to it

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 25, 2022
@SteveDunn
Copy link
Contributor

Hi @davidfowl - I've created a PR for this. I had a question:

Currently, a ConfigurationKeyNameAttribute can be applied to properties. Do we want this to also now be applicable to parameters?

Also, my implementation picks the biggest non-parameterless constructor and binds as many parameters as it can. I don't know how smart we need to be for this, but this seems like a reasonable starting point.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Mar 28, 2022
@Eli-Black-Work
Copy link

Will this change also make it so that configuration can now be bound to a class that has a non-empty constructor? 🙂

e.g.

public class User
{
    public string Username { get; }
    public string Password { get; }

    public User(string username, string password)
    {
        Username = username ?? ArgumentNullException.ThrowIfNull(username);
        Password = password ?? ArgumentNullExeption.ThrowIfNull(password);
    }
}

@SteveDunn
Copy link
Contributor

Yes, @Bosch-Eli-Black - that, and records with primary constructors

@SteveDunn
Copy link
Contributor

@davidfowl - you mentioned JSON in this description, so in the PR, we've borrowed some ideas from S.T.J. What are your thoughts on this thread in the PR?

@Eli-Black-Work
Copy link

Yes, @Bosch-Eli-Black - that, and records with primary constructors

@SteveDunn Great, thanks! 🙂

@maryamariyan maryamariyan self-assigned this May 31, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 9, 2022
@SteveDunn
Copy link
Contributor

So very pleased to see this feature done. It'll be useful to a lot of people. And it's been great to be able to contribute to a library that's used by so many people. Big thanks to the reviewers for steering such a feature to completion.

@davidfowl
Copy link
Member Author

SteveHarveyHappyGIF

@silkfire
Copy link

silkfire commented Jun 9, 2022

So we can expect this feature to be shipped in .NET 7? 😊

@davidfowl
Copy link
Member Author

Yes

@Eli-Black-Work
Copy link

Thanks a ton for the implementation, @SteveDunn ! 🙂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.