1574: Introducing AnnotateWith with values.#2792
Conversation
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.
…fore this can be considered done.
…it re-usable for other Gems.
…pping' for meta annotations.
|
@filiphr , do we want to call the additional arguments (key/value pairs) that you can place at an annotation, |
|
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. |
|
@Zegveld I added some initial findings. Need to do a real review though. Will be back for more thorough analysis. |
|
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 |
… 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
left a comment
filiphr
left a comment
There was a problem hiding this comment.
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?
|
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. |
|
I forgot the |
…r annotation elements of type 'Class<? extends Enum>'
…sed component processors
|
@Zegveld one last thing, I saw that |
I think we can remove that one, as it gives the same error as |
|
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 |
|
@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. |
|
@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. |
|
Thanks a lot for your final changes @Zegveld. I've squashed and integrated this into main. |
4 participants
introducing
@AnnotateWithfor adding custom annotations to classes or methods.closes #1574