-
Notifications
You must be signed in to change notification settings - Fork 70
[#253] option addGeneratedAnnotation for RecordBuilder #254
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: master
Are you sure you want to change the base?
Conversation
| .addAnnotation(Override.class).addAnnotation(generatedRecordInterfaceAnnotation) | ||
| .returns(ClassName.get(component.element.getReturnType())).addModifiers(Modifier.PUBLIC) | ||
| .addCode("return $L();", component.alternateName.get()).build(); | ||
| .addAnnotation(Override.class).returns(ClassName.get(component.element.getReturnType())) | ||
| .addModifiers(Modifier.PUBLIC).addCode("return $L();", component.alternateName.get()).build(); |
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.
This was not meant to be in that commit, will do the InterfaceProcessor later.
| * generated files. | ||
| * </p> | ||
| */ | ||
| GeneratedAnnotation addGeneratedAnnotation() default GeneratedAnnotation.ALWAYS; |
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.
Let's use a boolean here instead.
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.
So, not giving a choice for later options, like IF_ON_CLASSPATH or USE_USER_SUPPLIED_ANNOTATION? Just want to check. Booleans are restrictive, no choice of extending the options later on!
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.
We already have an option for "if on classpath" (that should be respected, BTW) so that ship has already sailed. Also, the use of the enum is very cumbersome. Let's use a boolean please.
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 haven't seen such an option. How can I enabled the "Generated" annotation already? It's not on the classpath and it got generated.
If we could make that working, let's just drop by this PR and issue.
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.
@Randgalt I have looked for this option but did not find it.
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.
@Randgalt that is about the RecordBuilderGenerated which does not need more than 2 states (add, do not add).
The issue is different with @Generated, because we could have it added conditionally. Because of this, I still recommend going with the ENUM approach.
| DISABLED, ENABLED, ENABLED_WITH_NULLABLE_ANNOTATION, | ||
| } | ||
|
|
||
| enum GeneratedAnnotation { |
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.
If we use this enum I'd like to think of better names. ALWAYS and NEVER are limiting. We will have trouble add another values that works with these. Some alternatives:
ENABLED/DISABLEDINCLUDE/EXCLUDE
Also, we could name the method generatedAnnotationMode or something similar.
| switch (metaData.addGeneratedAnnotation()) { | ||
| case ALWAYS -> builder.addAnnotation(generatedRecordBuilderAnnotation); | ||
| case NEVER -> { | ||
| /* no-op */ } | ||
| } |
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.
Let's move this to a utility as it's always the same. The utility might have ctors/factories for different builder types.
Refs: #253
Fixes: #253
TODO: