RenameLocalVariablesToCamelCase: Do not rename method argument names. #205
Conversation
…est parameter names or json field names. By renaming these without knowing how the frameworks use these we can make breaking changes. It's better to do no change.
joanvr
left a comment
joanvr
left a comment
There was a problem hiding this comment.
I'm not a big fan of removing the method parameters capability of the recipe, but for now seems the most sensitive thing to do if we cannot narrow it a bit more based on som anotations...
| rewriteRun( | ||
| java( | ||
| """ | ||
| @Controller |
There was a problem hiding this comment.
Are those anotations needed for the test? They are not imported anyway, thus giving incomplete type attribution... maybe we could remove them for simplicity.
There was a problem hiding this comment.
I added them to show give a more in-depth example of why this is added. I bet most people here recognise which annotations I mean with this.
I did not think the type attribution was worth adding spring as a dependency though.
Although perhaps I should change it to just have an @Issue annotation that explains the why?
There was a problem hiding this comment.
Note: Soon we will flag these missing type attributions in before sources as errors (unless the before type validation is configured to ignore these).
There was a problem hiding this comment.
Yes, please, reference this issue from the test too. As Knut says, we have plans to fail tests that do not have proper type attribution in the before step...
add issue
| } | ||
|
|
||
| @SuppressWarnings("JavadocDeclaration") | ||
| @Disabled |
There was a problem hiding this comment.
@joanvr did you notice this? I've disabled this test since it fixed an issue that might be important if we ever decide to start (limited) support for renaming method args.
There was a problem hiding this comment.
Maybe we could try to rename param references in annotations too? Although I'm not sure if we can have a generic enough approach
There was a problem hiding this comment.
Maybe, but even if there is 1 annotation, it does not mean that it's always used only with that annotation. It's a slippery slope and there will likely still be cases where we could break something.
Many frameworks depend on method argument names to use for request parameter names or json field names. By renaming these without knowing how the frameworks use these we can make breaking changes. It's better to do no change.
What's changed?
Non camel case names are kept in method argument names.
What's your motivation?
Do no harm
Anything in particular you'd like reviewers to focus on?
isMethodArgument()Anyone you would like to review specifically?
@joanvr @timtebeek
Have you considered any alternatives or workarounds?
no
Any additional context
Checklist
./gradlew licenseFormat