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

codegen-java: Support not annotating constructor parameters #792

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

odenix
Copy link
Contributor

@odenix odenix commented Nov 8, 2024

Motivation:
Spring Boot configuration classes neither require nor benefit from annotating constructor parameters with their name. The same is true for pkl-config-java configuration classes compiled with -parameter.

Changes:

  • add a noParamsAnnotation parameter to CLI, Gradle plugin, CliJavaCodeGeneratorOptions, and JavaCodegenOptions
  • default to annotating constructor parameters unless generateSpringBootConfig is set
  • fail if both paramsAnnotation and noParamsAnnotation are set
  • add tests
  • update docs

Result:
Generated code does not contain unnecessary annotations.

@odenix odenix force-pushed the codegen-java-params branch from dc475d5 to a06c301 Compare November 12, 2024 19:41
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; makes sense!

Looking for at least one more thumb from @stackoverflow or @holzensp.

Example: `paramsAnnotation = "org.project.MyAnnotation"` +
Fully qualified name of the annotation type to use for annotating constructor parameters with their name. +
The specified type must have a `value` parameter of type `String` or the generated code may not compile. +
This option cannot be combined with `noParamsAnnotation`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filling these in!

Comment on lines 163 to 165
Default: (flag not set unless `--generate-spring-boot` is set) +
Flag that indicates not to annotate constructor parameters with their name. +
This option cannot be combined with `--params-annotation`.
Copy link
Contributor

@bioball bioball Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add a blurb explaining how to use this. Same blurb should be copied to the pkl-gradle docs

Suggested change
Default: (flag not set unless `--generate-spring-boot` is set) +
Flag that indicates not to annotate constructor parameters with their name. +
This option cannot be combined with `--params-annotation`.
Default: (flag not set unless `--generate-spring-boot` is set) +
Flag that indicates not to annotate constructor parameters with their name. +
This option cannot be combined with `--params-annotation`.
If `true`, requires that Java is compiled with the `-parameters` flag, or that `-generate-spring-boot` is set, and classes are loaded by Spring Boot.

@odenix
Copy link
Contributor Author

odenix commented Nov 13, 2024

Today I discovered a valid reason not to generate @NonNull annotations, which begs the question if multiple --no- flags/APIs are the right solution.

My original attempt was to change paramsAnnotation = null to mean "no annotation" and set the default paramsAnnotation = "org.pkl.config.java.mapper.Named". However, I discovered a few downsides of this approach:

  • the default logic has to be duplicated in every layer (CLI/Gradle/CliGenerator/Generator)
  • --params-annotation=null isn't great (perhaps --params-annotation= could work?)
  • explicitly setting paramsAnnotation = null changes meaning (breaking change)

But perhaps this is nevertheless a better approach? I'd really like to support disabling both --params-annotation and --non-null-annotation, and I'm hoping to add --nullable-annotation.

@odenix odenix marked this pull request as draft November 13, 2024 01:57
@bioball
Copy link
Contributor

bioball commented Nov 13, 2024

What is the reason to not generate @NonNull?

@odenix
Copy link
Contributor Author

odenix commented Nov 13, 2024

I’d like to support @Nullable, and I think many users will want one or the other.

@bioball
Copy link
Contributor

bioball commented Nov 13, 2024

Gotcha. The reason we went with @NonNull instead of @Nullable is because we only need to use one annotation, rather than two (@Nullable implies a @NonNullByDefault type of annotation too).

I'd be happy to accept a PR for this, though.

Copy link
Contributor

@stackoverflow stackoverflow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@odenix odenix force-pushed the codegen-java-params branch from a06c301 to 4f7912d Compare November 13, 2024 23:56
@odenix
Copy link
Contributor Author

odenix commented Nov 14, 2024

After doing more research, I've come to the conclusion that disabling annotations with --params-annotation=none and paramsAnnotation = null is a better approach. I believe the same approach should be used for --non-null-annotation and a future --nullable-annotation. Please have another look. See commit message for details.

@odenix odenix force-pushed the codegen-java-params branch from 4f7912d to 58cd968 Compare November 14, 2024 00:40
@odenix odenix marked this pull request as ready for review November 14, 2024 00:40
@odenix odenix force-pushed the codegen-java-params branch 6 times, most recently from 3fd0783 to 39d2195 Compare November 14, 2024 01:03
Motivation:
Spring Boot configuration classes neither require nor benefit from annotating constructor parameters with their name.
The same is true for pkl-config-java configuration classes compiled with `-parameter`.

Changes:
- Change CLI parameter `--params-annotation` to accept a `none` value.
  This is recommended in https://clig.dev/#arguments-and-flags and is how the `-F` parameter of the `ssh` command works.
- Change `paramsAnnotation` property in Gradle plugin, CliJavaCodeGeneratorOptions, and JavaCodegenOptions as follows:
  - Change meaning of `null` from "generate org.pkl.java.config.mapper.Named annotations" to "do not generate annotations".
    This is a breaking change (only) affecting users who explicitly set the property to `null` instead of omitting it.
  - Change property default from `null` to:
    `null` if `generateSpringBootConfig` is `true` and `org.pkl.java.config.mapper.Named` otherwise
- add tests
- update docs of this and other codegen options

Result:
Generated code does not contain unnecessary annotations.
@odenix odenix force-pushed the codegen-java-params branch from 39d2195 to b091f21 Compare November 14, 2024 01:05
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.

3 participants