Skip to content

Conversation

@sjaakd
Copy link
Contributor

@sjaakd sjaakd commented Feb 25, 2019

I'll adapt the commit name later to an issue (to be written, or refer to the old issue for which I provided extra unit test)... But have a look..

There are things I forgot in the 2 step mapping which assure this goes right. It seems that rather than using conversion / built in directly, we only use the source-return types and than still resolve the method. That makes sure it works.. I probably need to add some additional comment to make this clear.

@sjaakd sjaakd requested a review from filiphr February 25, 2019 09:11
@filiphr
Copy link
Member

filiphr commented Feb 25, 2019

We don't really have to add an issue. Looks OK to me (tests are failing due to missing classes).

Why not just put everything in the top @WithClasses, like it was before?

@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 25, 2019

Why not just put everything in the top @WithClasses, like it was before?

Hard do debug.. All mappers come by + all mapping methods in each test case.

@filiphr
Copy link
Member

filiphr commented Feb 25, 2019

That is completely valid, I agree with you

Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 25, 2019

thanks for reviewing..

@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 25, 2019

now to convince travis 😄

@sjaakd sjaakd merged commit 6c1108d into mapstruct:master Feb 25, 2019
@sjaakd sjaakd deleted the extra_test branch September 14, 2019 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants