Added recipe for changing Groovy params

[ThomasKianek-Gepardec]

What's changed?

Added recipe for changing Groovy parameter value in method invocation.

What's your motivation?

To be used in Jenkins files (e.g. to update the JDK version).

Have you considered any alternatives or workarounds?

Preconditions for selecting a given method in Groovy did not work as expected (had to use if-statement instead).

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[sgartner03]

Hi @timtebeek, I've incorporated the suggested changes from the GH Action. I think this contribution matches your code styling now.

[sgartner03]

@ThomasKianek-Gepardec @Xaeras

[timtebeek]

hi @ThomasKianek-Gepardec & @sgartner03 ! Do I understand this is a recipe that can change a named parameter value in a Groovy method invocation to a different String value? I feel like we might be more explicit about those limitations in how we name the recipe, as opposed to only have that in the description.

It might also make sense to add some additional tests with methods that have more than one named parameter, only changing the right one, or no named parameters, just to ensure we handle all cases correctly.

[sgartner03]

Hi @timtebeek, yes exactly that is what this recipe does. Thank you for your, feedback, I'll incorporate it now.

[sgartner03]

Hi, I've added 3 more tests. This tests:

• Multiple params
• Empty Methods
• Parameters of other type.

As of now, this recipe only supports Strings and GStrings

[sgartner03]

@timtebeek

[timtebeek]

hi @sgartner03 ; I was out hiking a few days, hence the delayed response. I've pushed up a small formatting for the tests, and posted a number of questions on the implementation that I think we should address before a merge. A wider context of intended use might help for this otherwise generic recipe. I see now that this is intended to be used for Jenkins projects. Perhaps we can start this as a visitor in rewrite-jenkins first and let it mature from there?

[timtebeek]

What's the intended use case here? Because we'd much rather match using a MethodMatcher as opposed to a String comparision, as this would match any method anywhere of the same name, which is not as type safe as we prefer to be. For Gradle build script type information isn't always available to match reliably, but even then there's established patterns to be a little more sure we're not just matching any method by simple name, hence why I ask here.

[timtebeek]

This is the only time I've seen printTrimmed used in all of rewrite-groovy; is there no safer alternative? It seems you're looking to skip over anything that's not a J.Literal or GString; perhaps that can be checked here?

[sgartner03]

Yeah, you're right. I've removed extractQuoting

[timtebeek]

See below as well for a potential alternative looking at the type of the mapEntry instead, but calling the same method twice with the same argument is a bit suspect.

Suggested change

	char quote = extractQuoting(mapEntry);
	if (quote != '\'' && extractQuoting(mapEntry) != '"') {
	char quote = extractQuoting(mapEntry);
	if (quote != '\'' && quote != '"') {

[timtebeek]

Would we not want to retain any original formatting and comments by using withValue and withValueSource instead?

[sgartner03]

Hey @timtebeek ! Thank you for your extensive feedback. This is really valuable for me - so far I've only developed InHouse-Recipes for one Repo, where I didn't focus on clean code a lot.

Yes, you're right, this is intended for Jenkinsfiles on our end. We thought, it could be used in other Groovy Use-Cases too.

Initially, we thought we could decide between GString / J.String by using instanceof on mapEntry#getValue. However, we did notice that through all our tests, only J.Strings exist. Meaning, ", as well as ' quotes are represented by J.String. But I've removed extractQuoting() and replaced it by filtering through J.Literal#getType and just replacing the old value inside of J.Literal#getValueSource with the new one. This also replaced the constructor-call with just ".with*"-Operations.

However, I'm not that sure about using a MethodMatcher. I have to point out, that my groovy knowledge is close to zero. But how can types even be used in terms of stuff like a Jenkinsfile or other groovy stuff? You've mentioned other established ways to limit the scope, what would these be? And can't limiting the scope be implemented with a precondition on the user's end?

(PS I didn't mean to stress - I just forgot to ping you on the initial message)