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 for value converter attribute #408

Merged
merged 3 commits into from Jul 23, 2017

Conversation

Projects
None yet
2 participants
@tkouba
Contributor

tkouba commented Jul 21, 2017

This closes #401 and add simple support for value converter defined by attribute. Real value converters is not included, so integration test was not added.

@pleb

This comment has been minimized.

Show comment
Hide comment
@pleb

pleb Jul 23, 2017

Member

Thanks for the PR @tkouba

My only concern is that this will only work with the convention mapping method. Not a big issue, however, I'd like a few days to see if we can't implement in the old mapping methods. I'll keep the #401 open until I can get some time to look at it.

The pain points for introducing into the old mapping method will be modifying the IL generated code. The Umbraco devs have told me that the IL gen code can be removed without much of an impact on pref. Maybe it's time.

I'll merge the PR which will put it dev and the pre-release nuget channel.

Member

pleb commented Jul 23, 2017

Thanks for the PR @tkouba

My only concern is that this will only work with the convention mapping method. Not a big issue, however, I'd like a few days to see if we can't implement in the old mapping methods. I'll keep the #401 open until I can get some time to look at it.

The pain points for introducing into the old mapping method will be modifying the IL generated code. The Umbraco devs have told me that the IL gen code can be removed without much of an impact on pref. Maybe it's time.

I'll merge the PR which will put it dev and the pre-release nuget channel.

@pleb pleb merged commit 14b7919 into CollaboratingPlatypus:development Jul 23, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment