Implement ShouldUseConstructor #2698

lxalln · 2018-06-23T16:20:37Z

Allows for constructors to be excluded in the same way as properties
and fields (ShouldMapField & ShouldMapProperty).
Fixes #2691.

Allows for constructors to be excluded in the same way as properties and fields (ShouldMapField & ShouldMapProperty).

dnfclas · 2018-06-23T16:20:47Z

All CLA requirements met.

lxalln · 2018-06-23T16:23:54Z

I think I got all the places where ShouldMapField & ShouldMapProperty are used.

Couldn't find a sensible place for the tests to live, so I just put them with the big random list of tests in the bugs folder.

I'm not sure why ProfileMap is being shown as such a massive diff on GitHub, it really isn't a big diff.

Happy to make changes if required.

lbargaoanu · 2018-06-24T03:09:27Z

src/AutoMapper/Configuration/IProfileConfiguration.cs

+        /// Specify which constructors should be mapped.
+        /// By default all non-static constructors are mapped.
+        /// </summary>
+        Func<ConstructorInfo, bool> ShouldMapCtor { get; }


I think ShouldUseConstructor is a better name.

Agreed, I used Ctor because other places in AM use Ctor in place of Constructor.

Yes, an oversight :)

lbargaoanu · 2018-06-24T03:14:19Z

docs/Configuration.md

+## Global property/field/constructor filtering

-By default, AutoMapper tries to map every public property/field. You can filter out properties/fields with the property/field filters:
+By default, AutoMapper tries to map every public property/field, and all non-static declared constructors. You can filter out properties/fields/constructors with the property/field/constructor filters:


I think the wording is strange, AM doesn't map constructors, it uses them to construct destination objects. Also there is a special page for constructors, so the content should be there, not here.

I'll move it, I agree.

lbargaoanu · 2018-06-24T03:15:57Z

src/AutoMapper/ProfileMap.cs

-        }
-
-        private void BuildTypeMap(IConfigurationProvider configurationProvider, ITypeMapConfiguration config)
+using System;


I know GitHub sometimes messes the diff, but if you could fix it somehow, that would be nice :)

I'll see what I can do. I am assuming it's line endings, but no idea why it would affect only this file.

I have force saved the file with CRLF line endings, which has produced a much smaller diff in GitHub.

I am guessing that this file has messed up line endings. Not sure what I can do beyond this. It's at least reviewable now, even if 90% of the changes are not really changes.

lbargaoanu · 2018-06-24T03:17:41Z

src/AutoMapper/TypeDetails.cs

                        return !property.IsStatic() && shouldMapProperty(property);
                    case FieldInfo field:
                        return !field.IsStatic && shouldMapField(field);
+                    case ConstructorInfo constructor:


This doesn't belong here, constructors are not members to map.

lbargaoanu · 2018-06-24T03:20:35Z

src/AutoMapper/TypeMap.cs

            var constructors = DestinationTypeToUse
                .GetDeclaredConstructors()
-                .Where(ci => !ci.IsStatic);
+                .Where(ci => !ci.IsStatic && Profile.ShouldMapCtor(ci));


This should just check Constructors in TypeDetails.

It should have done that in the first place too 😄 I'll change it.

lbargaoanu · 2018-06-24T03:28:50Z

src/UnitTests/Bug/ShouldMapCtor.cs

+
+namespace AutoMapper.UnitTests.Bug
+{
+    public abstract class ShouldMapCtor


Or you can simply rename the namespace from Bug to ShouldMapCtor and then this class is not useful anymore. The file should be one level up. The tests should work against the API. Either assert that some constructor is used when calling Map or assert that configuration validation fails.

I'll move it.

I wasn't comfortable with these tests, but couldn't get past the way I'd tried to work around outside of AM. Asserting the configuration validation fails, should be fine. I'll do that.

Squash this commit out

…hould-map-ctor

lxalln · 2018-06-24T10:18:39Z

Right, we're stepping on each other's toes now. I'll leave this alone. I noticed the cosmetic changes too :)

Happy to squash it down if needs be.

…hould-map-ctor

lbargaoanu · 2018-06-24T10:19:42Z

@jbogard I think ShouldUseConstructor makes more sense than ShouldMapConstructor.

lbargaoanu · 2018-06-24T10:20:52Z

Sorry about the commits :) I thought it would easier to do it myself.

lxalln · 2018-06-24T10:21:53Z

No problem.

I agree with ShouldUseConstructor, unfortunately there is already DisableConstrutorMapping, so does that mean we should stick with Map instead of Use?

lbargaoanu · 2018-06-24T10:23:47Z

No, constructor mapping is only about mapping the constructor parameters from the source object. You're filtering all the constructors, regardless of constructor mapping.

lbargaoanu · 2018-06-24T10:50:35Z

@lxalln Can you please do the rename, as I don't have VS handy?

lxalln · 2018-06-24T10:52:11Z

Will do shortly

lbargaoanu · 2018-06-24T11:32:55Z

I was wrong, this will still allow a constructor without parameters to be used, regardless of the condition. I'd say we want it to apply to those too, but I guess it's debatable.

lbargaoanu

Thanks.

lxalln · 2018-06-25T09:13:52Z

@lbargaoanu The tests I put in prove that an empty constructor cannot be used if it is being excluded by the filter. Am I missing something?

lbargaoanu · 2018-06-25T09:15:58Z

If there is only the default constructor, it will be used regardless of the condition, right?

lxalln · 2018-06-25T09:21:50Z

The default constructor is returned from GetDeclaredConstructors and will be filtered like any other constructor. If that means there are no constructors, then it will fail validation.

lbargaoanu · 2018-06-25T09:25:10Z

Make this test. The condition always returns false. You have only a default constructor. The mapping works. It's not unreasonable. We'l just have to see what people expect :)

lock · 2019-05-05T19:33:20Z

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.

lxalln added 2 commits June 23, 2018 17:19

Implement ShouldMapCtor

ce88ab2

Allows for constructors to be excluded in the same way as properties and fields (ShouldMapField & ShouldMapProperty).

Update documentation for ShouldMapCtor

bde8375

lbargaoanu suggested changes Jun 24, 2018

View reviewed changes

lxalln and others added 6 commits June 24, 2018 10:46

Updates for PR review

19757a7

Squash this commit out

Switch to CRLF for ProfileMap

ed90dcd

cosmetic

552229f

Remove extraneous comment

8992f47

cosmetic

118d1ea

Merge branch 'should-map-ctor' of github.com:lxalln/AutoMapper into s…

b8ff085

…hould-map-ctor

Merge branch 'should-map-ctor' of github.com:lxalln/AutoMapper into s…

3e0679b

…hould-map-ctor

Rename ShouldMapConstructor to ShouldUseConstructor

f463a47

rename

0e32388

lbargaoanu approved these changes Jun 24, 2018

View reviewed changes

Rename ShouldMapConstructor.cs to ShouldUseConstructor.cs

1c2a06f

lbargaoanu changed the title ~~Implement ShouldMapCtor~~ Implement ShouldUseConstructor Jun 24, 2018

cosmetic

cd9d962

jbogard added the Feature label Jun 25, 2018

jbogard added this to the v.Next milestone Jun 25, 2018

jbogard merged commit 88a7c78 into LuckyPennySoftware:master Jun 25, 2018

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

Implement ShouldUseConstructor #2698

Implement ShouldUseConstructor #2698

Uh oh!

Conversation

lxalln commented Jun 23, 2018 • edited by lbargaoanu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lxalln commented Jun 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lxalln commented Jun 24, 2018

Uh oh!

lbargaoanu commented Jun 24, 2018

Uh oh!

lbargaoanu commented Jun 24, 2018

Uh oh!

lxalln commented Jun 24, 2018

Uh oh!

lbargaoanu commented Jun 24, 2018

Uh oh!

lbargaoanu commented Jun 24, 2018

Uh oh!

lxalln commented Jun 24, 2018

Uh oh!

lbargaoanu commented Jun 24, 2018

Uh oh!

lbargaoanu left a comment

Choose a reason for hiding this comment

Uh oh!

lxalln commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbargaoanu commented Jun 25, 2018

Uh oh!

lxalln commented Jun 25, 2018

Uh oh!

lbargaoanu commented Jun 25, 2018

Uh oh!

lock bot commented May 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lxalln commented Jun 23, 2018 •

edited by lbargaoanu

Loading

dnfclas commented Jun 23, 2018 •

edited

Loading

lxalln commented Jun 25, 2018 •

edited

Loading