Skip to content

Support binding to immutable types #43662#67161

Closed
SteveDunn wants to merge 11 commits intodotnet:mainfrom
SteveDunn:support-binding-to-immutable-types
Closed

Support binding to immutable types #43662#67161
SteveDunn wants to merge 11 commits intodotnet:mainfrom
SteveDunn:support-binding-to-immutable-types

Conversation

@SteveDunn
Copy link
Contributor

@SteveDunn SteveDunn commented Mar 25, 2022 •
edited
Loading

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 community-contribution Indicates that the PR has been added by a community member area-Extensions-Configuration labels 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: -

eerhardt
eerhardt reviewed Mar 28, 2022
View reviewed changes
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs Outdated
Copy link
Member

@eerhardt eerhardt Mar 28, 2022

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?

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs Outdated
Copy link
Member

@eerhardt eerhardt Mar 28, 2022

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?

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated
Copy link
Member

@eerhardt eerhardt Mar 28, 2022

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:

runtime/src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationKeyNameAttribute.cs

Lines 8 to 9 in bd43f55

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

@SteveDunn SteveDunn force-pushed the support-binding-to-immutable-types branch from 4bf28bc to 15bf080 Compare March 28, 2022 18:54
@SteveDunn SteveDunn force-pushed the support-binding-to-immutable-types branch from 15bf080 to 9d70502 Compare March 28, 2022 19:06
@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

SteveDunn commented Mar 28, 2022

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

SteveDunn commented Mar 28, 2022

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

SteveDunn commented Mar 28, 2022

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

@SteveDunn SteveDunn mentioned this pull request Mar 28, 2022
Merged
@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.

Reviewers

@davidfowl davidfowl davidfowl left review comments

@eerhardt eerhardt eerhardt left review comments

@jeffhandley jeffhandley Awaiting requested review from jeffhandley

@marek-safar marek-safar Awaiting requested review from marek-safar

@lambdageek lambdageek Awaiting requested review from lambdageek

@vargaz vargaz Awaiting requested review from vargaz

@lateralusX lateralusX Awaiting requested review from lateralusX

@BrzVlad BrzVlad Awaiting requested review from BrzVlad

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@thaystg thaystg Awaiting requested review from thaystg

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky

Assignees

No one assigned

Labels

area-Extensions-Configuration community-contribution Indicates 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.

Support immutable types with configuration binding

3 participants

@SteveDunn @davidfowl @eerhardt