-
Notifications
You must be signed in to change notification settings - Fork 101
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a standard Matcher
for this I think.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better style to use plugin configuration
.
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.
Better style
Do you have a reference to a style guide?
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.
Could not find one. I wrote up what I know: https://stackoverflow.com/a/67794133/12916
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
MethodParameters
first (the most efficient and supported system) then fall back to using ASM forLocalVariableTable
if 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.