Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Support TypeConverterAttribute on bound properties #787

Closed
wants to merge 5 commits into from
Closed

Support TypeConverterAttribute on bound properties #787

wants to merge 5 commits into from

Conversation

williamb1024
Copy link

Addresses Issue #622

Checks for TypeConverterAttribute on current property before falling back to TypeDescriptor.GetConverter().

The majority of the changes involve passing the current PropertyInfo between the various methods.

@natemcmaster natemcmaster changed the base branch from dev to master July 2, 2018 17:44
@ajcvickers
Copy link
Contributor

@williamb1024 Sorry for taking so long to get to this. We reviewed this PR again today and while we believe adding support for type converters in configuration is a worthwhile thing to do we have some concerns. Specifically, this needs to be done in such a way that it will work consistently with all the configuration providers, and we need to have confidence that this is the case--historically, we have found that it is easy to miss places when adding support for type converters. So the PR, as it stands, is missing test coverage at least.

In addition we have recently consolidated the configuration code into the https://github.com/aspnet/Extensions repo. Unfortunately, it is not trivial to move PRs over to another repo. If you wish to move forward with this contribution we would ask you to re-create the PR in the new repo complete with additional test coverage and any other changes needed as revealed by the tests. We apologize again for making this hard.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants