Add AddNullMethodArgument recipe
#4047
Conversation
When changing a method we often add a nullable argument. Sometimes overriding it as to not break existing consumers. With this recipe we can migrate more quickly by adding a null value as the argument.
| List<String> parameterNames = new ArrayList<>(methodType.getParameterNames()); | ||
| parameterNames.add(argumentIndex, "null"); | ||
| List<JavaType> parameterTypes = new ArrayList<>(methodType.getParameterTypes()); | ||
| parameterTypes.add(argumentIndex, JavaType.Primitive.Null); |
There was a problem hiding this comment.
I'm not so sure about this parameter type Null here; Perhaps we need another argument where folks can pass in a FQN of the target type. That way any subsequent recipes in a larger migration will also match the method invocation going forward, especially if this is one partial step of a larger migration.
By comparison, now we end up with this after a recipe execution in void addToMiddleArgument()
There was a problem hiding this comment.
ah, now this makes a lot more sense indeed. I did not fully understand what this part was doing to be honest 😅
I think passing the type (maybe also param name?) is OK as you should always know the exact method you are migrating to.
I don't think there is a way to determine/find this data if the method is not defined in the project is there?
There was a problem hiding this comment.
Not as easily reliably no, especially when we start stacking migration recipes where you start out without that new method available in an older version of a library.
| } | ||
|
|
||
| args.add(argumentIndex, new J.Literal(randomId(), args.isEmpty() ? Space.EMPTY : Space.SINGLE_SPACE, Markers.EMPTY, "null", "null", null, JavaType.Primitive.Null)); | ||
| m = m.withArguments(args); |
There was a problem hiding this comment.
Perhaps this would be a little more in line with what we use elsewhere?
m = m.withArguments(ListUtils.insert(m.getArguments(), new J.Literal(...), argumentIndex));
There was a problem hiding this comment.
I copied most of the cose (including this part) from DeleteMethodArgument but happy to change to this!
rewrite-java-test/src/test/java/org/openrewrite/java/AddNullMethodArgumentTest.java
Outdated
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/AddNullMethodArgumentTest.java
Outdated
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/AddNullMethodArgumentTest.java
Outdated
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/AddNullMethodArgumentTest.java
Outdated
Show resolved
Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/AddNullMethodArgument.java
Outdated
Show resolved
Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/AddNullMethodArgument.java
Outdated
Show resolved
Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/AddNullMethodArgument.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Tim te Beek <tim@moderne.io> Co-authored-by: Shannon Pamperl <shanman190@gmail.com>
| } | ||
|
|
||
| @Test | ||
| void addCastToConflictingArgumentType() { |
There was a problem hiding this comment.
I've added this because of possible conflicts. However I'm not sure how to get the cast to only use the not-so-fully qualified name.
There was a problem hiding this comment.
I think this is fine; should be rare that there's overloads, which require an explicit cast to String.
If you really want to get rid of this you can shorted fully qualified type references as we do here:
https://github.com/openrewrite/rewrite-templating/blob/e69126c2209f4edd1cb8e203d6eaeda26ff744b1/src/main/java/org/openrewrite/java/template/internal/AbstractRefasterJavaVisitor.java#L45
There was a problem hiding this comment.
I tried this but it does not shorten the cast.
There was a problem hiding this comment.
For java.lang we indeed don't shorten fully qualified names, as that could be wrong.
There was a problem hiding this comment.
Although note that also java.lang types would end up as unqualified if the compilation unit already contains unqualified references to that type (which is often the case), so using this visitor is probably still a good idea.
|
@shanman190 Thanks, not sure how I missed those 🙈 |
| } | ||
|
|
||
| @Test | ||
| void addCastToConflictingArgumentType() { |
There was a problem hiding this comment.
I think this is fine; should be rare that there's overloads, which require an explicit cast to String.
If you really want to get rid of this you can shorted fully qualified type references as we do here:
https://github.com/openrewrite/rewrite-templating/blob/e69126c2209f4edd1cb8e203d6eaeda26ff744b1/src/main/java/org/openrewrite/java/template/internal/AbstractRefasterJavaVisitor.java#L45
What's changed?
Add recipe
AddNullMethodArgumentWhat's your motivation?
When changing a method we often add a nullable argument. Sometimes overriding it as to not break existing consumers. With this recipe we can migrate more quickly by adding a null value as the argument.
Anything in particular you'd like reviewers to focus on?
line 104 of the recipe, creating the actual
nullliteral.AlsoaddArgumentsConsecutivelyis not working as expected. Any suggestions? If not I'll give it another try soon.Anyone you would like to review specifically?
@timtebeek
Checklist