Add Advanced property and put BeforeSeal function in it #1883

Merged
merged 4 commits into from Jan 11, 2017

Projects

None yet

3 participants

@TylerCarlson1
Member

As discussed in #1866

@lbargaoanu
Contributor

A test would help.

@lbargaoanu
Contributor

Maybe it's just me, but I don't understand the test :) Maybe it's even correct, but I cannot tell by reading the code.

@TylerCarlson1
Member

Yea there's no easy to read that the map was being called before seal was called. Hence why I didn't write a test in the first place. I didn't add an IsSealed property or anything like that cause it serves no purpose other than to try to prove this test correct.

Seal builds the _typemapcahce, and when you try to resolve a type map it will check there. What the test shows is that if you check it on before seal there's no type map in cache, even though you just created a map. And I even added the same function call outside Mapper.Initialize to show that there's a difference in result and that it is sealed.

@lbargaoanu
Contributor

I think the extra test only confuses things. Less is more here.

@lbargaoanu
Contributor
lbargaoanu commented Jan 5, 2017 edited

Maybe add another flag to prove the method was called at all. So assert it was called and that it was called before seal.

@TylerCarlson1
Member

Oh yea you are right I didn't even notice that. By default _sealed is false and doesn't really prove the method was called.
I can make _sealed nullable, so you know it got set.

@lbargaoanu
Contributor

It makes sense.

src/AutoMapper/AdvancedConfiguration.cs
+ /// <summary>
+ /// Action called against the IConfigurationProvider before it gets sealed
+ /// </summary>
+ public Action<IConfigurationProvider> BeforeSeal { get; set; }
@jbogard
jbogard Jan 5, 2017 Member

This doesn't allow extensions to stack more than one action. I think you make this a method BeforeSeal(Action<IConfigurationProvider>)

@TylerCarlson1
TylerCarlson1 Jan 5, 2017 Member

Yea I thought about that. But I got the thing to work with AM.Collections with a single action, so haven't changed it yet.

src/AutoMapper/MapperConfiguration.cs
@@ -37,7 +37,8 @@ public MapperConfiguration(MapperConfigurationExpression configurationExpression
Configuration = new ProfileMap(configurationExpression);
Profiles = new[] { Configuration }.Concat(configurationExpression.Profiles.Select(p => new ProfileMap(p, configurationExpression))).ToArray();
- configurationExpression.Advanced.BeforeSeal?.Invoke(this);
+ foreach (var beforeSealAction in configurationExpression.Advanced.BeforeSealActions)
+ beforeSealAction?.Invoke(this);
@lbargaoanu
lbargaoanu Jan 6, 2017 Contributor

I think this code should be inside Advanced and then you don't need to expose the list.

@TylerCarlson1
TylerCarlson1 Jan 6, 2017 Member

Everything else goes through a foreach loop of items in this class. Like profiles you can see the property but to add you call add function, so what's the difference? It's exposed as an enumerable as to not add another way. I don't see changing it as "that" big of a deal in exposing it or not.

@lbargaoanu
lbargaoanu Jan 6, 2017 Contributor

It's encapsulated, that's the difference. An event does this for example.

@TylerCarlson1
Member

Is this good @jbogard or is the before seal action implementation blocking it?

@jbogard
Member
jbogard commented Jan 11, 2017

I might do a wee bit of refactoring here but I don't want to block this one.

@jbogard jbogard merged commit 0f29418 into AutoMapper:master Jan 11, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jbogard jbogard added the Feature label Jan 11, 2017
@jbogard jbogard added this to the vNext milestone Jan 11, 2017
@TylerCarlson1 TylerCarlson1 deleted the TylerCarlson1:AdvancedBeforeSeal branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment