-
Notifications
You must be signed in to change notification settings - Fork 394
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 a default closure in InterceptingGCL #510
Generate a default closure in InterceptingGCL #510
Conversation
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.
There is a lot of logic going on in defaultClosure
that isn't tested. It would be nice to have a unit test to make sure that all of the string slicing and appending works as expected.
Also, there is a typo in the commit message (it should be "Generate a default ..."). I fixed this in the PR title for you.
src/main/groovy/com/lesfurets/jenkins/unit/InterceptingGCL.groovy
Outdated
Show resolved
Hide resolved
In case a method is matched with null value closure, the default generated closure is {}. This does not work when the method has more parameters, so the approach of generating a default closure using dynamic compilation was taken.
Also adds a verification for the maximum JVM allowed parameters.
e4240a1
to
db9c32d
Compare
@nre-ableton more unit tests are now in place. |
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 guess this change looks fine to me, but I am not sure I can speak to the broader impact that this change may have. It would be nice if @stchar could also take a look.
src/main/groovy/com/lesfurets/jenkins/unit/InterceptingGCL.groovy
Outdated
Show resolved
Hide resolved
Co-authored-by: Nik Reiman <[email protected]>
Hi @stchar, can you please also take a look at this pull request? Thanks! |
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.
Awesome!
@nestoracunablanco, @nre-ableton, reviewed. Looks good. |
Thanks for the nice feedback! |
In case a method is matched with null value closure, the default generated closure is {}.
This does not work when the method has more parameters, so the approach of generating a default closure using dynamic compilation was taken.
Solves comments in issue #461