Add mapToMultiple mapping operation

mark-gerarts / automapper-plus

An AutoMapper for PHP

MIT License

551 stars 30 forks source link

Add mapToMultiple mapping operation #70

Closed peter-si closed 2 years ago

peter-si commented 3 years ago

Add polymorphic mapping opperation from https://github.com/mark-gerarts/automapper-plus/issues/64

peter-si commented 3 years ago

Hi @mark-gerarts just a friendly reminder since you mentioned you will take a look

mark-gerarts commented 3 years ago

I'm sorry for the delay. I've done some peer review on the code - looking good, just a few small remarks! The only thing left for me to do is cloning the repo and playing around with the PR, doing some manual testing. Once I've done that and once you've had a chance to take a look at the comments, I'll be more than happy to merge this.

Thanks for your work!

peter-si commented 3 years ago

Hi, sorry it took me so long, but I've finally updated it. Most of it should be fixed (I just didn't know where you want that Readme part:). I'm not used to these Github PR (I'm a Gitlab user) and I'm not sure if you see how the code changed regarding your comments (I noticed that outdated flag, but it doesn't look enough for me). Anyway take a look at it, hopefully it will be ok ;)

Also is there anything needed to be changed in AutoMapperPlusBundle in order to get these changes? I think it should work out of the box after update? But maybe some configs? :man_shrugging:

peter-si commented 3 years ago

Hi, @mark-gerarts have you had time to check this out?

fd6130 commented 2 years ago

Still waiting this PR news.

mark-gerarts commented 2 years ago

@fd6130 thanks for the reminder, some of these things have been gathering dust for too long.

@peter-si sorry for getting back to you so late. I refactored some minor things, but most importantly, I renamed the feature from MapToMultiple to MapToAnyOf. I feel this is a more correct naming because of several reasons:

MapToMultiple sounds more like it deals with mapping to a collection of things, even though it can be a single property as well.
MapToOneOf as suggested in the related issue was disregarded there as well, since this implies the entire collection is of one type.
MapToAnyOfis the name of this PR's branch and is used here and there in the tests, and sounds right to me.

I'll merge this into master, but will wait a bit before tagging a release after I've done some spring cleaning on open issues etc (poke me if this takes too long and I'll tag a new release regardless).

mark-gerarts commented 2 years ago

...and forgot to answer this:

Also is there anything needed to be changed in AutoMapperPlusBundle in order to get these changes? I think it should work out of the box after update? But maybe some configs?

Yes, this will work out-of-the-box after a composer update mark-gerarts/automapper-plus-bundle --with-all-dependencies or equivalent.

Thanks again for the PR @peter-si!