-
Notifications
You must be signed in to change notification settings - Fork 92
Do not adopt Arrays.toString for varargs arguments #246
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
Conversation
|
@philippe-granet do these changes align with what you expected after logging |
@timtebeek yes it's OK for me, thanks! |
| if (methodType == null) { | ||
| return mi; | ||
| } | ||
| List<JavaType> parameterTypes = methodType.getParameterTypes(); |
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.
An alternative would be to rely on Flag.Varargs which when set on the JavaType.Method means that it is a varargs method.
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.
Didn't know that flag, thanks! But then that only applies to the entire method right? Yet we'd still want to do replacements on String.format and friends when we can. Say String.format gets called with 6 arguments, one of them is an array. The current implementation would then still wrap that one array in Arrays.toString. Or how would you apply that flag here?
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.
Not sure. I would probably just use the flag to check what the recipe should do with the arguments corresponding to the (last) varargs parameter, if any. Probably no huge improvement...
Fixes #230
What's changed
In this proposal we look at the method parameter type, and if that is also an array we do not produce a change.
Anything in particular you'd like reviewers to focus on?
Not sure I actually like this change yet; It seems before we were too eager to make changes, even where we changed to logic. The recipe was intended not to show the result of
arr.toString(), that that wasn't what happened on some of the previous cases. Take for exampleNote
How we print
s=a, nots=[Ljava.lang.String;@73a8dfccas would be the case forargs.toString()That means that the recipe might inadvertently in the old situation changes the above to start producing
s=[a, b, c]forString.format("s=%s", Arrays.toString(args))