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

Remove commons-lang3 #94

Merged
merged 1 commit into from
Apr 13, 2020
Merged

Conversation

timja
Copy link
Member

@timja timja commented Apr 9, 2020

@timja
Copy link
Member Author

timja commented Apr 9, 2020

Ran a basic smoke test which seemed to work, the library was loaded:

library 'pipeline'
buildPlugin()

@dwnusbaum
Copy link
Member

dwnusbaum commented Apr 9, 2020

@timja Thanks for filing the PR! I think that normal cases should work fine, my main concern is if @jglick had some particular reason not to use InvokerHelper. Maybe it was to avoid potential sandbox issues if the static method call is a meta method or category method or some other Groovy-specific feature?

@jglick
Copy link
Member

jglick commented Apr 9, 2020

if @jglick had some particular reason not to use InvokerHelper

To the extent I can recall now, I think I just did not realize it was available.

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Thanks for filing a PR for this!

@@ -265,16 +263,10 @@ public AutoCompletionCandidates doAutoCompleteIdentifier(@AncestorInPath ItemGro
@Override public Object invokeMethod(String name, Object _args) {
Class<?> c = loadClass(prefix + clazz);
Object[] args = _args instanceof Object[] ? (Object[]) _args : new Object[] {_args}; // TODO why does Groovy not just pass an Object[] to begin with?!
Copy link
Member

Choose a reason for hiding this comment

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

nit: InvokerHelper handles this conversion internally via InvokerHelper.asArray, so I think this line could be removed now.

@dwnusbaum dwnusbaum merged commit c27ef48 into jenkinsci:master Apr 13, 2020
@timja timja deleted the remove-commons-lang3 branch April 13, 2020 15:29
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.

5 participants