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

Generate metadata for reflection on method parameters #415

Merged
merged 1 commit into from
May 30, 2021

Conversation

basil
Copy link
Member

@basil basil commented May 29, 2021

See jenkinsci/stapler#226 and the Maven documentation. Once widely adopted, this would allow us to use reflection rather than ASM in Stapler for discovering method parameters. To test this change, I verified that a plugin was not able to access method parameters via reflection with the current plugin parent POM but was able to access them from a plugin parent POM with these changes. I also verified that plugins can opt out of this functionality if desired by setting maven.compiler.parameters to false in their <properties> section.

timja
timja approved these changes May 30, 2021
@timja timja merged commit aff15ac into jenkinsci:master May 30, 2021
@basil basil deleted the parameters branch May 30, 2021 14:01
@oleg-nenashev
Copy link
Member

Thanks @basil ! Released it. Jenkins POM is also in progress

@@ -95,6 +95,9 @@
<scmTag>HEAD</scmTag>
<!-- Where to put temporary files during test runs. -->
<surefireTempDir>${project.build.directory}/tmp</surefireTempDir>

<!-- Generate metadata for reflection on method parameters -->
<maven.compiler.parameters>true</maven.compiler.parameters>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you did not do this in <configuration> of the plugin? Is there some expectation that some plugins would want to opt out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason you did not do this in <configuration> of the plugin?

Is there a reason I should have?

Is there some expectation that some plugins would want to opt out?

No expectation of that, but I thought it best to provide an escape hatch should this cause some unexpected problem.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason I should have?

Just generally better style to typed configuration that is explicitly scoped to a particular mojo, unless there is a specific expectation that the property will be overridden sometimes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an action item for me here?

Copy link
Member

Choose a reason for hiding this comment

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

Not really.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll take this as a note for future work then. I'm happy to redo this if you think there is a need, but right now it seems good enough.

alextu added a commit to alextu/gradle-jpi-plugin that referenced this pull request Oct 28, 2024
alextu added a commit to jenkinsci/gradle-plugin that referenced this pull request Dec 10, 2024
alextu added a commit to jenkinsci/gradle-plugin that referenced this pull request Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants