Reduce ASM dep in ClassDescriptor
#233
Conversation
| assertNotNull("Method missing for " + entry.getKey(), testMethod); | ||
| String[] result = ClassDescriptor.loadParameterNamesFromReflection(testMethod); | ||
| assertNotNull("Null result for " + entry.getKey(), result); | ||
| if (!Arrays.equals(entry.getValue(), result)) { |
There was a problem hiding this comment.
Could use a standard Matcher for this I think.
There was a problem hiding this comment.
I was just trying to change the existing code as little as possible. This is pretty much a copy of loadParametersFromAsm. There is a lot that can be cleaned up in this repository. Once the dependencies are up-to-date I'll start looking into cleanups like this.
There was a problem hiding this comment.
In particular the use of reflection in the ASM test is unnecessary (fixed in #226).
pom.xml
Outdated
| @@ -60,6 +60,9 @@ | |||
| <changelist>999999-SNAPSHOT</changelist> | |||
| <scmTag>HEAD</scmTag> | |||
| <spotbugs.excludeFilterFile>${project.basedir}/../src/spotbugs/spotbugs-excludes.xml</spotbugs.excludeFilterFile> | |||
|
|
|||
| <!-- Generate metadata for reflection on method parameters --> | |||
| <maven.compiler.parameters>true</maven.compiler.parameters> | |||
There was a problem hiding this comment.
Better style to use plugin configuration.
There was a problem hiding this comment.
Better style
Do you have a reference to a style guide?
There was a problem hiding this comment.
Could not find one. I wrote up what I know: https://stackoverflow.com/a/67794133/12916
There was a problem hiding this comment.
OK, this still seems like a fairly subjective argument to me, but I trust your judgment.
|
LMK if you intend to do any follow-ups; otherwise this can be merged as is. |
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
pom.xml
Outdated
| @@ -89,6 +89,14 @@ | |||
| </ignores> | |||
| </configuration> | |||
| </plugin> | |||
| <plugin> | |||
| <groupId>org.apache.maven.plugins</groupId> | |||
There was a problem hiding this comment.
By the way, for official plugins this can be omitted if you like:
| <groupId>org.apache.maven.plugins</groupId> |
or kept for legibility.
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
See stapler/stapler#226 (comment). Amends that PR to try
MethodParametersfirst (the most efficient and supported system) then fall back to using ASM forLocalVariableTableif necessary. Tested interactively in Jenkins core in the following scenarios:In the first scenario I verified that ASM was correctly used as a fallback. In the second scenario I verified that ASM was not used as a fallback.