Value transformers in global, profiles, type maps, and members #2261

jbogard · 2017-08-09T15:28:56Z

No description provided.

jbogard · 2017-08-09T15:31:37Z

TylerCarlson1 · 2017-08-09T16:05:12Z

src/AutoMapper/Execution/TypeMapPlanBuilder.cs

@@ -522,6 +522,10 @@ private Expression BuildValueResolverFunc(PropertyMap propertyMap, Expression de
                    );
            }

+            valueResolverFunc = _typeMap.Profile.ValueTransformers
+                .Where(vt => vt.IsMatch(propertyMap))
+                .Aggregate(valueResolverFunc, (current, vtConfig) => ToType(ReplaceParameters(vtConfig.TransformerExpression, ToType(current, vtConfig.ValueType)), propertyMap.DestinationPropertyType));


Can this change to having ValueTransformerConfiguration have function likeMapExpression? Kinda like what IObjectMapper does now where it's an interface like IValueTransformerConfiguration.

Yeah that's a further task on the list

I wasn't going to make it like IObjectMapper though, more like IValueResolver and ITypeConverter. You can go the expression route, which can be used in LINQ etc. or the runtime route that can get access to ResolutionContext. That would also allow a DI-able version.

jbogard · 2017-08-10T03:41:59Z

This one's ready to review.

TylerCarlson1 · 2017-08-14T02:34:24Z

What do you think about making ValueTransformerConfiguration an interface with that being an implementation of it. That way it's more versatile to transform things in the future. Also people will be able to make their own custom ones based on whatever they want if they so desire.

The only issue with this if you want to keep the same public function calls ApplyTransform is should probably be an extension method and you would have to supply the List of transforms so the extension method can access and add to the list.

Unless this is meant for another PR in the future, then can discuss it there.

jbogard · 2017-08-14T20:23:15Z

Hrm that's a big departure from the rest of the configuration API. I think you already have the ability to do similar though with ForAllTypeMaps?

TylerCarlson1 · 2017-08-15T00:06:43Z

But for all type maps doesn't apply to individual property maps and can't be profile specific. That's the whole reason for transforms.

Also your current implementation is only for property maps of same type. What about different types being mapped what about comparing by some kind of naming convention. If you keep the API as it is for each one of them you'll have to keep adding more and more functions to the interface, as well as their implementation in multiple places away from the base class. Where as if you use extension methods they can be confined to their own classes near their respected implementations of IValueTransformerConfiguration for profiles and member configurations.

ValueTransformerConfiguration can have a static extensions class in the same location as it, maybe even a sub class in the same file, with all the method calls that use it. Instead of every configuration interface having an implementation in 2-3 separate files that you have to search for if you want to change something.

Yes it's a big change in how the API is structured, but one thing I don't like about working with the current one is that the MapperConfiguration, Profile, and everything else have interfaces that have like 20-30 properties/functions that do almost the exact same things using a class and keep adding functionality with more and more function calls is going to make navigating them even worse to wade through.

I think this makes the code more easily expandable, without having to hack around in private properties or submit PR's and wait for the next version if you want to do something extra. But then it exposes things that you might not want the user to worry about in configuration. It's a trade off, the big one being change how API is structured, but from the end user experience I don't think it will change, unless there's some weird thing between Extension methods that I'm not aware of.

jbogard · 2017-08-15T01:50:12Z

PR for a PR? I can see some benefits to the flexibility for sure, I do get a little wary of just extension methods since they aren't nearly as discoverable. I would be interested in a concrete example of what you're suggesting though.

…

On Mon, Aug 14, 2017 at 7:06 PM, Tyler Carlson ***@***.***> wrote: But for all type maps doesn't apply to individual property maps and can't be profile specific. That's the whole reason for transforms. Also your current implementation is only for property maps of same type. What about different types being mapped what about comparing by some kind of naming convention. If you keep the API as it is for each one of them you'll have to keep adding more and more functions to the interface, as well as their implementation in multiple places away from the base class. Where as if you use extension methods they can be confined to their own classes near their respected implementations of IValueTransformerConfiguration for profiles and member configurations. ValueTransformerConfiguration can have a static extensions class in the same location as it, maybe even a sub class in the same file, with all the method calls that use it. Instead of every configuration interface having an implementation in 2-3 separate files that you have to search for if you want to change something. Yes it's a big change in how the API is structured, but one thing I don't like about working with the current one is that the MapperConfiguration, Profile, and everything else have interfaces that have like 20-30 properties/functions that do almost the exact same things using a class and keep adding functionality with more and more function calls is going to make navigating them even worse to wade through. I think this makes the code more easily expandable, without having to hack around in private properties or submit PR's and wait for the next version if you want to do something extra. But then it exposes things that you might not want the user to worry about in configuration. It's a trade off, the big one being change how API is structured, but from the end user experience I don't think it will change, unless there's some weird thing between Extension methods that I'm not aware of. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGYMhKtu2iHBRCeigmYVu2hDhv0Z-02ks5sYOETgaJpZM4OyP7O> .

lbargaoanu · 2017-08-10T06:42:52Z

src/AutoMapper/Execution/TypeMapPlanBuilder.cs

@@ -522,6 +522,12 @@ private Expression BuildValueResolverFunc(PropertyMap propertyMap, Expression de
                    );
            }

+            valueResolverFunc = propertyMap.ValueTransformers
+                .Concat(_typeMap.ValueTransformers)
+                .Concat(_typeMap.Profile.ValueTransformers)


There might be a lot of transformers here, so a cache would help.

I don't know much about transformers, but that sounded weird :)

A few hundred mappings, maybe a few thousand properties, it seems reasonable. For all the existing transformers. This will hurt eventually :)

What would we cache? The list could be different since you can define at the property map level.

Alternatively we could push the transformers down from the parent levels, kinda like we do for Include.

Yes, but not the transformers per member are the problem, all the rest are. And yes, I thought about smth like your second suggestion, but I'm not sure how that would look like, caching by type seemed more obvious.

I think this will only be an issue with large amounts of value transformers. And I'm not sure that would really happen

I guess we can wait :)

lbargaoanu · 2017-08-10T06:47:09Z

src/AutoMapper/ValueTransformerConfiguration.cs

+
+        public bool IsMatch(PropertyMap propertyMap)
+        {
+            return ValueType.IsAssignableFrom(propertyMap.DestinationPropertyType);


Maybe the other way around?

lbargaoanu · 2017-08-10T06:53:53Z

src/AutoMapper/IMemberConfigurationExpression.cs

+        /// </summary>
+        /// <typeparam name="TValue">Value type to match and transform</typeparam>
+        /// <param name="transformer">Transformation expression</param>
+        void ApplyTransform<TValue>(Expression<Func<TValue, TValue>> transformer);


where TValue : TDestination? Although integer conversions don't fit.

lbargaoanu · 2017-08-10T06:56:27Z

src/AutoMapper/ValueTransformerConfiguration.cs

+
+namespace AutoMapper
+{
+    public class ValueTransformerConfiguration


I guess this could be a struct.

lbargaoanu · 2017-08-10T07:04:06Z

src/UnitTests/ValueTransformers.cs

+            {
+                cfg.CreateMap<Source, Dest>();
+                cfg.ApplyTransform<string>(dest => dest + " is straight up dope");
+                cfg.ApplyTransform<string>(dest => dest + "! No joke!");


So the order matters here. I don't know about that...

I'm guessing some people expect stacking and some expect overriding :)

Maybe the wording can be better? But yes, they'd stack is the idea.

I think it's easy to make a mistake. You have a lot of config code and you have one ApplyTransform and a few pages below, oops, another one. Not exactly the pit of success :)

I think AddTransform would be better and exposing a list even clearer.

I see your point, but I don't mind

cfg.ValueTransformers.Add<string>(dest => dest + "!!!");

It is two dots. But it makes more obvious what's going on. I'm not sure if you wanted to do other things with that list.

I've been following this at a little bit of a distance. FWIW, from a functionality standpoint I like the idea of stacking transforms, but it seems like a departure from the rest of the API, which I think just does overrides. On the whole I think I would vote for ApplyTransform() and overrides, rather than AddTransform() and stacking, but I don't feel strongly about it.

@lbargaoanu I gotcha, add the extension method to the list of value transformers rather than the root. I don't want to create a custom collection type.

@asherber I think we could add conditions to transforms that would allow for turning things off (or don't apply things globally).

@jbogard @lbargaoanu I kinda like extending off the list better. Less mass options from Intelisense and groups functionality by property.

Also can make it fluent where you add and then return list, so you can stack, and know you are stacking.

OK I made some changes here lemme know what you think. I couldn't go extension methods all the way because it would have required forcing generic arguments or a weird chaining.

lbargaoanu · 2017-08-28T14:21:52Z

src/AutoMapper/IMemberConfigurationExpression.cs

+        /// </summary>
+        /// <typeparam name="TValue">Value type to match and transform</typeparam>
+        /// <param name="transformer">Transformation expression</param>
+        void AddTransform<TValue>(Expression<Func<TValue, TValue>> transformer);


Maybe a non generic overload that just uses TMember.

I don't know if we need an overload as much as just get rid of this version. I don't remember why I put a transform where you could specify a different type. Transformers get applied after resolving/mapping so it's always going to be of the destination member type.

After resolving and before mapping. And I think you meant for TValue to be the type of the resolved value. Which doesn't have to be TMember. Right? :)

Ah then I wonder if I have value transformations in the right place. I think it's really before you assign it, you transform it but I have it as part of resolving.

I moved the value transformations to after mapping and right before setting.

I like it. I've always thought AfterPropertyMap is a good idea :)

Ha well maybe this will take care of people's scenarios.

jbogard · 2017-09-05T07:30:21Z

Anything else on this one?

asherber · 2017-09-05T12:27:44Z

Thank you!

lock · 2019-05-05T23:33:13Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jbogard added 2 commits August 8, 2017 22:07

Basic config options

3a0b4fe

Basic transforming

a98c14b

jbogard added the Feature label Aug 9, 2017

jbogard added this to the 6.2.0 milestone Aug 9, 2017

jbogard mentioned this pull request Aug 9, 2017

Value Transformers #2258

Closed

6 tasks

TylerCarlson1 reviewed Aug 9, 2017

View reviewed changes

jbogard added 5 commits August 9, 2017 11:36

Inherting value transformers from root config

159918f

Adding test for value types

14a1c52

Removing console writelines

cfdf4f8

Transformers in type maps

1b5a7be

Value transformers at the member level

32ec282

jbogard changed the title ~~[WIP] Value transformers in profiles~~ Value transformers in global, profiles, type maps, and members Aug 10, 2017

lbargaoanu requested changes Aug 15, 2017

View reviewed changes

TylerCarlson1 mentioned this pull request Aug 16, 2017

[Example] Moving functionality to ExtensionMethods #2279

Closed

AutoMapper deleted a comment from TylerCarlson1 Aug 16, 2017

Going with exposing config directly and extension methods

03c70e3

lbargaoanu reviewed Aug 28, 2017

View reviewed changes

jbogard added 2 commits September 5, 2017 00:59

Applying changes requested

8e58d72

Moving value transformers from value resolving step to post-mapping step

e54b53d

lbargaoanu approved these changes Sep 5, 2017

View reviewed changes

jbogard merged commit 01a4010 into AutoMapper:master Sep 5, 2017

AutoMapper deleted a comment from jbogard Sep 5, 2017

lock bot locked as resolved and limited conversation to collaborators May 5, 2019

Value transformers in global, profiles, type maps, and members #2261

Value transformers in global, profiles, type maps, and members #2261

Conversation

jbogard commented Aug 9, 2017

jbogard commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbogard commented Aug 10, 2017

TylerCarlson1 commented Aug 14, 2017

jbogard commented Aug 14, 2017

TylerCarlson1 commented Aug 15, 2017

jbogard commented Aug 15, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbogard commented Sep 5, 2017

asherber commented Sep 5, 2017

lock bot commented May 5, 2019