-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add support for repeatable annotations #1670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Hello! The build is failing because of formatting violations. You can check them locally by running |
I made the fix and confirmed it works locally. |
Hello, thanks for the PR! I might have missed something here, but why did you create a second implementation for the annotations? Is there a reason we couldn't add the functionality for repeated annotations into the |
This comment was marked as resolved.
This comment was marked as resolved.
After thinking more about your previous comment, I realized there might be a better approach that allows You can see it in this commit — I've confirmed that it doesn't break any existing tests: Sorry if my earlier proposal ended up taking unnecessary time. Do you think this approach would be better? |
Yes, please update the PR. Thanks! |
I've updated the PR. @mhalbritter |
* @return {@code true} if an annotation with the specified name is present, otherwise | ||
* {@code false} | ||
*/ | ||
public boolean has(String name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like the concept of an annotation name. What do you think about deprecating the add
methods, and add two new methods: addSingle
and addRepeatable
.
addSingle
behaves essentially like add
but if there is more than one annotation of the given class and a customizer is provided it fails. Also addSingle
doesn't add a annotation if it's already there.
addRepeatable
can't customize already existing annotations, it always adds a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought AnnotationContainer
needed a key to identify specific annotations when there are multiple ones of the same type.
But you're probably right that the annotation name concept adds unnecessary complexity.
So you're suggesting addSingle
/addRepeatable
would be cleaner because most use cases don't need to handle specific instances of repeatable annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the existing add
method somewhat confusing in that it either adds or modifies the existing one. That behavior is not applicable to repeatable annotations.
We could also think about adding:
addSingle
(fails if there is already one of the same type)addRepeatable
addSingleOrCustomize
(essentially the existingadd
method - but fails if there are multiple annotations of that type registered already. Allows to customize the annotation if there is one already).
I have to play around a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more data point to split the functionality from add
and customize
: io.spring.initializr.generator.buildsystem.gradle.GradleConfigurationContainer
has separate add
and customize
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, io.spring.initializr.generator.buildsystem.gradle.GradlePluginContainer
also combines add
and customize
. Hm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, customize
might be a fine name too.
From a ClassName
perspective, customize
and replace
seem to have similar meanings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I think this is much cleaner than the previous implementation.
I found that single annotations have customize functionality, but repeatable annotations don't.
Do you think repeatable annotations should also have customize features like single annotations do?
If so, methods like customizeAllRepeatable
, customizeRepeatableIf
, or removeRepeatableIf
might be helpful for more granular control over repeatable annotations.
What do you think? Should I look into adding these features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at it. Do you want to take this code into your PR?
I don't think customizing repeatable annotations make sense. We can add this functionality later, if the need arises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll update the PR based on the code you provided and mention you.
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Signed-off-by: sijun-yang <[email protected]>
Resolves issue #1624 by adding support for multiple annotations of the same type.
Key changes:
AnnotationContainer
to support multiple annotationsKotlinSourceCodeWriter#writeProperty
to correctly write annotationsFixes gh-1624