1574: Introducing AnnotateWith with values. #2792

Zegveld · 2022-03-18T19:23:45Z

introducing @AnnotateWith for adding custom annotations to classes or methods.

Still need to implement the following: 1. succesful situations for all types except class and string. 2. succesful situations for all types with multiple values (array format). 3. add compilation error when multiple value types are filled. 4. check if behavior works correct for incorrect type with array format.

… expected in annotation.

…arget types.

…fore this can be considered done.

…it re-usable for other Gems.

…pping' for meta annotations.

Zegveld · 2022-03-18T19:25:25Z

@filiphr , do we want to call the additional arguments (key/value pairs) that you can place at an annotation, properties or parameters?
I noticed that currently I'm using both. The annotation has @Parameter while the code talks about properties.

tthornton3-chwy · 2022-05-06T19:24:15Z

@filiphr @Zegveld this change would help us greatly in a couple of places, is there any movement or anything needed for this one?

filiphr · 2022-05-06T19:26:03Z

This is a really good improvement indeed @tthornton3-chwy. I've been pretty busy with other activities and haven't had the chance to review it properly. I hope that I'll be able to give a proper pass soon.

core/src/main/java/org/mapstruct/AnnotateWith.java

processor/src/main/java/org/mapstruct/ap/internal/model/annotation/BooleanProperty.java

sjaakd · 2022-06-01T07:00:00Z

@Zegveld I added some initial findings. Need to do a real review though. Will be back for more thorough analysis.

Zegveld · 2022-06-01T17:53:15Z

Did quite a bit of renaming. Might need another round of renames, but at least it's getting closer to the java spec ;) instead of just key or Property/parameters :) Resolving the old remarks concerning renames.

… import the internal anotations, but mention the containing class at the '@GemDefinition' annotation.

* Remove TargetGem and rely on it being present * Use Type#describe for the error messages * Remove findAllAnnotationParameters from the AdditionalAnnotationsBuilder and extract the needed information in the builder itself

filiphr

Really great job on this one @Zegveld.

I took the liberty to do some small polishing in a separate PR. Please have a look at it, and if you do not like it let me know. I felt like I could do a bit more in AdditionalAnnotationsBuilder and perhaps simplify certain things, move things around the checks of the annotation parameters in one method, but I didn't want to change a lot before talking to you.

I have also left some inline comments.

Some more general things that we need to clear out:

Enum API

The API around enums feels a bit strange. If you want to define multiple enums you will need to write:

@Element(name = "enumArray",
        enums = {
            @AnnotateWith.EnumElement(enumClass = AnnotateWithEnum.class, name = "EXISTING"),
            @AnnotateWith.EnumElement(enumClass = AnnotateWithEnum.class, name = "EXISTING")
        }
    )

Basically you'll need to define the enumClass twice. I think that we need to revisit this API.

e.g.

@Element( name = "enumArray", 
        enumValue = @AnnotateWith.EnumElement( 
            enumClass = AnnotateWithEnum.class,
            names = {
                "EXISTING",
                "EXISTING"
            }
        )
    ),

or

@Element( name = "enumArray",
        enumClass = AnnotateWithEnum.class,
        enumNames = {
            "EXISTING",
            "EXISTING"
        }
    )

With the second approach we can get rid of the @EnumElement.

What do you think?

Define same element multiple times

You can now write:

@AnnotateWith.Element( name = "stringParam", strings = "value1" ),
@AnnotateWith.Element( name = "stringParam", strings = "value2" ),

From what I could see in the code, we are going to pick value1 and nothing will happen with value2. I think that we should not allow this. Same as we are doing for Mapping#target.

What do you think?

core/src/main/java/org/mapstruct/AnnotateWith.java

processor/src/test/java/org/mapstruct/ap/test/annotatewith/DeprecateMapper.java

core/src/main/java/org/mapstruct/AnnotateWith.java

processor/src/test/java/org/mapstruct/ap/test/annotatewith/AnnotateWithTest.java

...r/src/main/resources/org/mapstruct/ap/internal/model/annotation/BooleanAnnotationElement.ftl

processor/src/main/resources/org/mapstruct/ap/internal/model/BeanMappingMethod.ftl

core/src/main/java/org/mapstruct/AnnotateWith.java

… error message

filiphr · 2022-06-06T10:42:29Z

I did a small pass over some of the errors @Zegveld let me know if you agree with it or not.

I also changed one from error to warning, because I thought that having an error for that is a bit too strict.

Zegveld · 2022-06-06T16:01:22Z

I did a small pass over some of the errors @Zegveld let me know if you agree with it or not.

I also changed one from error to warning, because I thought that having an error for that is a bit too strict.

changes look good, time to add fixture tests.

filiphr · 2022-06-06T19:17:47Z

I forgot the HiddenEnumForAnnotationDefault. Can we do this in the same way NullEnum is done in Junit Jupiter and put it in its own file with package-protected visibility.

…r annotation elements of type 'Class<? extends Enum>'

…sed component processors

filiphr · 2022-06-18T18:36:11Z

@Zegveld I did one more pass and add some small changes. Let me know if you do not agree with them.

I also saw how the generated annotations look like and I created #2895 so we can improve that a bit.

Zegveld · 2022-06-18T18:44:32Z

@Zegveld I did one more pass and add some small changes. Let me know if you do not agree with them.

Looking good to me.

I also saw how the generated annotations look like and I created #2895 so we can improve that a bit.

Sounds good.

filiphr · 2022-06-18T18:46:41Z

@Zegveld one last thing, I saw that ErroneousMapperWithTooManyParameterValues is not used anywhere. Are we missing some test case perhaps?

Zegveld · 2022-06-18T19:43:55Z

@Zegveld one last thing, I saw that ErroneousMapperWithTooManyParameterValues is not used anywhere. Are we missing some test case perhaps?

I think we can remove that one, as it gives the same error as ErroneousMapperWithWrongParameter. Or do we want a different error if there are too many parameters opposed to wrong parameter?
Then we need to add a test for it, and probably another compiler error message.

filiphr · 2022-06-18T19:58:13Z

I think that we need to provide a different compiler error message if they have defined multiple parameters. Similar like if you do it for the @Mapping

filiphr · 2022-07-08T19:04:13Z

@Zegveld should we do the last change for this one and then merge this to main?

Zegveld · 2022-07-25T14:30:17Z

@Zegveld should we do the last change for this one and then merge this to main?

Sure, I've been busy and haven't had time to work on the last changes yet. Feel free to pick it up otherwise I'll try to do it next weekend if I have some time then.

Zegveld · 2022-08-15T15:01:39Z

@filiphr could you take a look at the new error message. I feel like it isn't quite correct, but can't figure out what. If you think it is fine as it is, I think it can then be merged.
Unless there was something else that I forgot to do ofcourse ;)

filiphr · 2022-08-20T10:59:59Z

Thanks a lot for your final changes @Zegveld. I've squashed and integrated this into main.

Ben Zegveld added 14 commits March 7, 2022 23:54

mapstruct#1574: Support added for all singular forms of parameters.

39836d7

mapstruct#1574: Support added for array form of parameters.

70f89e9

mapstruct#1574: added an error for multiple values while only 1 value…

0fe8edb

… expected in annotation.

mapstruct#1574: added checks on annotations being placed on correct t…

2931d7f

…arget types.

mapstruct#1574: forgot to add additional test classes.

80933ae

mapstruct#1574 added comment to something that needs to be changed be…

14a6abe

…fore this can be considered done.

mapstruct#1574: added support for meta-annotations for

072a2b5

mapstruct#1574: moved meta annotation behavior to util class to make …

01083a9

…it re-usable for other Gems.

mapstruct#1574: Changed behavior to match '@mapping' and '@SubclassMa…

141ff6e

…pping' for meta annotations.

mapstruct#1574: refactoring to make it all fit together better.

1777f98

mapstruct#1574: fix build.

609f3b1

mapstruct#1574: replaced manual formatting with .ftl files.

56d4665

mapstruct#1574: add missing license header.

b9299b3

sjaakd reviewed Jun 1, 2022

View reviewed changes

core/src/main/java/org/mapstruct/AnnotateWith.java Outdated Show resolved Hide resolved

sjaakd reviewed Jun 1, 2022

View reviewed changes

processor/src/main/java/org/mapstruct/ap/internal/model/annotation/BooleanProperty.java Outdated Show resolved Hide resolved

sjaakd reviewed Jun 1, 2022

View reviewed changes

processor/src/main/java/org/mapstruct/ap/internal/model/annotation/BooleanProperty.java Outdated Show resolved Hide resolved

mapstruct#1574: Rename variables to better match java spec.

90bb493

Ben Zegveld and others added 6 commits June 4, 2022 01:52

mapstruct#1574: more renames and support for enums.

5878b6a

mapstruct#1574: missing javadoc and fixed long lines.

86268b8

mapstruct#1574: documentation, javadoc and adjust GemGenerator to not…

4f64413

… import the internal anotations, but mention the containing class at the '@GemDefinition' annotation.

Changes in the additional annotations builder

ea688a3

* Remove TargetGem and rely on it being present * Use Type#describe for the error messages * Remove findAllAnnotationParameters from the AdditionalAnnotationsBuilder and extract the needed information in the builder itself

Use ClassAssert from AssertJ for checking for annotations on a class

648dc91

Reformat AnnotateWithTest so Checkstyle doesn't complain

0faac4d

filiphr reviewed Jun 4, 2022

View reviewed changes

Duplicated identical annotation should be a warning + remove not used…

c7eec33

… error message

Ben Zegveld added 2 commits June 6, 2022 15:38

mapstruct#1574: added examples to javadoc. fixed line length.

7e76cd6

mapstruct#1574: remove paragraph tags

8dbb1f7

Ben Zegveld and others added 8 commits June 6, 2022 22:51

mapstruct#1574: converted the more complex tests into fixture tests.

f9ba5c1

mapstruct#1574: introducing a NullEnum util class as default class fo…

5c60c3d

…r annotation elements of type 'Class<? extends Enum>'

mapstruct#1574: fix javadoc

b8045a0

mapstruct#1574: fix line ending

9117141

mapstruct#1574: add missing license header.

5e834d6

Move NullEnum to be package protected in the org.mapstruct package

f823f79

Documentation and javadoc typo changes

2ebdc60

Simplify ConvertToProperty a bit

070b650

filiphr mentioned this pull request Jun 18, 2022

Generate more readable code for custom annotations #2895

Closed

Simplify some creation of the annotation element in the annotation ba…

4eb98a1

…sed component processors

filiphr approved these changes Jun 18, 2022

View reviewed changes

mapstruct#1574: Too many element parameters then specific error message.

701709f

filiphr mentioned this pull request Aug 17, 2022

#2773 feat: Copy the method annotation to the implementation method #2960

Merged

filiphr merged commit 849085e into mapstruct:main Aug 20, 2022

filiphr mentioned this pull request Apr 15, 2023

Evaluate @Nullable annotations to support combining mapstruct with uber/nullaway #3182

Open

Uh oh!

1574: Introducing AnnotateWith with values. #2792

1574: Introducing AnnotateWith with values. #2792

Uh oh!

Conversation

Zegveld commented Mar 18, 2022

Uh oh!

Zegveld commented Mar 18, 2022

Uh oh!

tthornton3-chwy commented May 6, 2022

Uh oh!

filiphr commented May 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sjaakd commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zegveld commented Jun 1, 2022

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Enum API

Define same element multiple times

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filiphr commented Jun 6, 2022

Uh oh!

Zegveld commented Jun 6, 2022

Uh oh!

filiphr commented Jun 6, 2022

Uh oh!

filiphr commented Jun 18, 2022

Uh oh!

Zegveld commented Jun 18, 2022

Uh oh!

filiphr commented Jun 18, 2022

Uh oh!

Zegveld commented Jun 18, 2022

Uh oh!

filiphr commented Jun 18, 2022

Uh oh!

filiphr commented Jul 8, 2022

Uh oh!

Zegveld commented Jul 25, 2022

Uh oh!

Zegveld commented Aug 15, 2022

Uh oh!

filiphr commented Aug 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sjaakd commented Jun 1, 2022 •

edited

Loading