Skip to content

Add custom name binding for .Net Configuration. #40422

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

Closed
wants to merge 3 commits into from

Conversation

Coderrob
Copy link
Contributor

@Coderrob Coderrob commented Aug 5, 2020

Took a swag at addressing the gap between the Json source and the Binder projects. The Json settings I work with contain names that don't match the setting classes. Those are mapped by JsonProperty or DataMember where a name is given that can be mapped.

The binder project, rightfully, doesn't know about Json or any other formats. The one area where it could be built up more is around looking at the attributes associated with the properties of the object it's attempting to bind to the configuration dictionary. This is an attempt to try to just that.

Summary of the changes (Less than 80 chars):

  • Added new BindingOption flag for binding by attribute names
  • Broke out the property value lookup into a standalone function
  • Added a function to return "name" values from custom attribute data from the PropertyType

Addresses issue #36010

Internal dictionary created from json/xml for configurations do not honor custom names on the JSON property bindings for class properties. This change adds support.
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Aug 6, 2020

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

@Coderrob
Copy link
Contributor Author

@maryamariyan Do you know what steps I should take for this PR? Seems like there is a CI/CD issue with installing required prerequisites for the failing tests.

@maryamariyan
Copy link
Member

maryamariyan commented Aug 10, 2020

If you are working on windows is a doc page on requirements for building on windows.

A bit more on building and testing.

From what I see in the CI results, there is an error on the test project. I think it will repro for you locally:

ConfigurationBinderTests.cs(221,31): error CS1061: (NETCORE_ENGINEERING_TELEMETRY=Build) 'BinderOptions' does not contain a definition for 'BindPropertyUsingAttributeName' and no accessible extension method 'BindPropertyUsingAttributeName' accepting a first argument of type 'BinderOptions' could be found (are you missing a using directive or an assembly reference?)

Also note since you are adding API, the issue page needs to represent API proposal, usage, and the API needs approval as per guideline in API review process. PRs cannot be merged or approved until the API is approved first. We are approaching the 5.0 code complete this week, so chances are this would most probably fall on the 6.0 timeframe because it involves a new API.

@Coderrob
Copy link
Contributor Author

Coderrob commented Aug 10, 2020

@maryamariyan patched - all the other builds could reference it and the runtime builds there were the only ones error on (NETCORE_ENGINEERING_TELEMETRY=InitializeToolset) Failed to install dotnet SDK from public location (exit code '1').

Does this count as a new API? It adds a boolean binding property to the BinderOptions to allow custom name binding from the extensions configuration library.

@davidfowl
Copy link
Member

Yes it's new API and it's a new feature

@Coderrob
Copy link
Contributor Author

Closing pending another issue to determine whether API is decided upon per request in issue #36010

@Coderrob Coderrob closed this Aug 11, 2020
@Coderrob Coderrob deleted the add-named-config-bindings branch August 11, 2020 19:35
@karelz karelz modified the milestones: 6.0.0, 5.0.0 Aug 18, 2020
@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.

6 participants