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 binding to immutable types #43662 #67161

Closed

Conversation

SteveDunn
Copy link
Contributor

@SteveDunn SteveDunn commented Mar 25, 2022

Fixes #43662

Update
I completely screwed up my git repo and this PR, so it was easier and cleaner to create a new branch and a new PR. This is at #67258.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 25, 2022
@ghost
Copy link

ghost commented Mar 25, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #43662

Author: SteveDunn
Assignees: -
Labels:

area-Extensions-Configuration, community-contribution

Milestone: -

@@ -112,6 +112,75 @@ public class DerivedOptionsWithIConfigurationSection : DerivedOptions
public IConfigurationSection DerivedSection { get; set; }
}

#if NETCOREAPP
Copy link
Member

Choose a reason for hiding this comment

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

Why is this only on NETCOREAPP?

@@ -112,6 +112,75 @@ public class DerivedOptionsWithIConfigurationSection : DerivedOptions
public IConfigurationSection DerivedSection { get; set; }
}

#if NETCOREAPP
public record RecordTypeOptions(string Color, int Length);
Copy link
Member

Choose a reason for hiding this comment

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

How about tests for readonly struct and record struct?

@@ -698,6 +746,37 @@ private static Array BindArray(Array source, IConfiguration config, BinderOption
options);
}

// todo: steve - we might not need this; currently, these attributes are only applicable to properties and
Copy link
Member

Choose a reason for hiding this comment

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

What's the plan for addressing this?

I agree that this code doesn't make sense because these attributes can't be on parameters:

[AttributeUsage(AttributeTargets.Property)]
public sealed class ConfigurationKeyNameAttribute : Attribute

@SteveDunn SteveDunn force-pushed the support-binding-to-immutable-types branch from 9d70502 to 9bf910f Compare March 28, 2022 19:18
@SteveDunn SteveDunn closed this Mar 28, 2022
@SteveDunn SteveDunn force-pushed the support-binding-to-immutable-types branch from 9bf910f to e32f3b6 Compare March 28, 2022 19:27
@SteveDunn
Copy link
Contributor Author

I had some git issues and somehow this PR was closed. Is there a way to reopen it? (cc @maryamariyan )

@eerhardt eerhardt reopened this Mar 28, 2022
@SteveDunn
Copy link
Contributor Author

Apologies @davidfowl @eerhardt - It's been one of those evenings and I completely screwed up my git repo and this PR and somehow accidentally closed this PR.

I created a new PR at #67258

@SteveDunn
Copy link
Contributor Author

@eerhardt - thanks for reopening - I force-pushed one too many times in this branch, and have 20 odd files from another branch I'm working on! I've blitzed this one and the new branch and PR is at #67258

Apologies again - I knew I should've called it a day a few hours back!

@eerhardt eerhardt closed this Mar 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 30, 2022
@SteveDunn SteveDunn deleted the support-binding-to-immutable-types branch June 4, 2022 07:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support immutable types with configuration binding
3 participants