feat: Add @AnnotateWith support to mapping methods by chenzijia12300 · Pull Request #2986 · mapstruct/mapstruct

@chenzijia12300

Fixes #2983
Enables @AnnotateWith to support NormalTypeMappingMethod and ValueMappingMethod

@chenzijia12300

@chenzijia12300

I don't really understand why the unit test failed, I shouldn't have affected Issue2897Test.shouldImportNestedClassInMapperImports to run
(⊙﹏⊙)

@chenzijia12300

@Zegveld

I don't really understand why the unit test failed, I shouldn't have affected Issue2897Test.shouldImportNestedClassInMapperImports to run (⊙﹏⊙)

Looking into this issue. Might be that something in the main branch is broken on that part. Seems to be related to how mapstruct handles automated imports and user defined imports.

Edit: created issue #2990 for this, fix already existed under #2984

@filiphr

It was my fault. Everything should be fixed now on main. I restarted the jobs

filiphr

Choose a reason for hiding this comment

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

The implementation looks good @chenzijia12300. I've left some comments for some small fixes there.

In addition to that, I've left some comments for the tests. Can we please move them a bit like I proposed?

@filiphr

Regarding the failing tests. I tried running the builds, but it seems like it doesn't really do a proper merge commit or something like that. Can you please rebase on top of main and push to your branch so that you get the latest changes?

@chenzijia12300

Thank you very much for your review @filiphr , I will try to implement your proposal at a later time

@chenzijia12300

I don't really understand why the unit test failed, I shouldn't have affected Issue2897Test.shouldImportNestedClassInMapperImports to run (⊙﹏⊙)

Looking into this issue. Might be that something in the main branch is broken on that part. Seems to be related to how mapstruct handles automated imports and user defined imports.

Edit: created issue #2990 for this, fix already existed under #2984

Thank you very much for your reply 😊 @Zegveld

@chenzijia12300

…ckstyle for StreamMappingMethod constructor methods

filiphr

Choose a reason for hiding this comment

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

Thanks a lot for your changes @chenzijia12300.

I've left some final comments that we should address before we merge this

@chenzijia12300

@chenzijia12300

Thanks a lot for your changes @chenzijia12300.

I've left some final comments that we should address before we merge this

I apologize for my carelessness and thank you for your comment, I have revised it

@filiphr

I apologize for my carelessness and thank you for your comment, I have revised it

Thanks a lot for your revisions. Everything is looking great now. I've merged this to main now. And no need to apologise, that's why we have PRs, so we can share knowledge and ideas.

@chenzijia12300