Skip to content
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

Add @Generated annotation to classes and includeGeneratedAnnotation config option #1202

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

cyclotomic18
Copy link

@cyclotomic18 cyclotomic18 commented Mar 1, 2021

Restores the feature which added @javax.annotation.Generated annotation to generated code. In addition, the feature becomes conditional on a boolean configuration property "markGenerated" (as in JAXB. openapi ecc...), defaulted to false.
Addresses #1162 and related, without falling back to #577.

@cyclotomic18
Copy link
Author

Imo, this feature is of extreme importance in a code-genereation tool like this, for the reason i mentioned in #835 (comment). Thus, i kindly ask for an asap minor including this feature, if and once it's approved and all good.

@joelittlejohn
Copy link
Owner

I'm actually tempted to turn this back on by default, with logic like:

  • add javax.annotation.Generated if it is available
  • if not, add javax.annotation.processing.Generated if it is available

This doesn't specifically use the target 'source compliance' version but it's a good compromise IMO. We are already moving to 1.1.0 for other reasons (and the versioning scheme in this project means that changes like this are expected).

@joelittlejohn
Copy link
Owner

Out of interest, are you using Java 8?

@cyclotomic18
Copy link
Author

cyclotomic18 commented Mar 1, 2021

No, I'm using java 11, but Spring Boot packs jackarta.annotation-api (which contains javax.annotation.Generated) so I have no issue about that.
However, I mostly agree with resolving the type of Generated the way you exposed...I just wanted to address the main issue, that is the current absence of a primary feature.
I'm not sure about turning it on by default, do you mean with no prop at all or by defaulting the prop to true? For sure I wouldn't go with the first solution; the second one is pretty much just a 50/50 choice, I think everyone is good as long as the feature is present in some way...maybe it's better to stay as clean as possible with a default/minimal configuration, though? Idk.

@joelittlejohn
Copy link
Owner

Yeah, I think we could keep the flag and have it true by default. This means in the CLI using something like --omit-generated-annotation as the flag (the presence of the flag sets it to false).

I realise you have matched jaxb, but in this project we usually name flags like this includeXXX (these control what is added to the ouput), so I'd prefer includeGeneratedAnnotation (default true) for all the other plugins.

Also, the value of the Generated annotation should be "jsonschema2pojo". The name of the generator.

@cyclotomic18
Copy link
Author

I made the changes you mentioned in your last comment

@joelittlejohn joelittlejohn changed the title Added markGenerated prop for (conditional) @Generated annotation Add @Generated annotation to classes and includeGeneratedAnnotation config option Mar 1, 2021
@joelittlejohn joelittlejohn added this to the 1.1.0 milestone Mar 1, 2021
@joelittlejohn
Copy link
Owner

Awesome. Could you combine your commits? I'll make some further changes to detect which generated annotation is available before 1.1.0 is released.

@cyclotomic18
Copy link
Author

I rebased with master and force-pushed.
When's 1.1.0 going to be released approximately?

@joelittlejohn
Copy link
Owner

@cyclotomic18 Soon I'd say. I expect it will be this week.

@joelittlejohn joelittlejohn merged commit 0d480f0 into joelittlejohn:master Mar 2, 2021
@cyclotomic18
Copy link
Author

Awesome, thank you for the support!

@cyclotomic18 cyclotomic18 deleted the mark-generated branch March 3, 2021 08:51
@joelittlejohn
Copy link
Owner

@cyclotomic18 1.1.0 is released now 👍

@cyclotomic18
Copy link
Author

Ye, already included in my project, all good to go now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants