Rename variables if the name matches class name #5454
Conversation
radoslaw-panuszewski
force-pushed
the
rename-variables-when-changing-type
branch
2 times, most recently
from
b83c9b8 to
071fb76
Compare
radoslaw-panuszewski
force-pushed
the
rename-variables-when-changing-type
branch
from
071fb76 to
8551095
Compare
|
|
||
| /** | ||
| * @return string with first character changed to lowercase (or empty string) | ||
| */ | ||
| public static String decapitalize(@Nullable String string) { | ||
| if (string != null && !string.isEmpty()) { | ||
| return Character.toLowerCase(string.charAt(0)) + string.substring(1); | ||
| } | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Could we start this out as just a private method inside ChangeType itself? We try to minimize exposed API, as folks will use anything they can see, which limits our ability to evolve for something essentially only used once so far.
There was a problem hiding this comment.
Moved to private method in ChangeType
| public A2 method(A2 a2) { | ||
| return a2; |
There was a problem hiding this comment.
Neat idea indeed to rename these as well!
| // Rename variable if it matches class name (starting with a lowercase character) | ||
| if (ident.getSimpleName().equals(decapitalize(className))) { | ||
| if (targetType instanceof JavaType.FullyQualified) { | ||
| String newName = decapitalize(((JavaType.FullyQualified) targetType).getClassName()); |
There was a problem hiding this comment.
Would you mind wrapping this in a use of VariableNameUtils.generateVariableName? in 99.9...% of the cases those will be the same, but it's best to avoid conflicts where we can.
There was a problem hiding this comment.
Done as suggested 😉
| */ | ||
| public static String decapitalize(@Nullable String string) { | ||
| if (string != null && !string.isEmpty()) { | ||
| return Character.toLowerCase(string.charAt(0)) + string.substring(1); |
There was a problem hiding this comment.
Pedantic mode: we should probably work on codepoints instead of characters since Java identifiers allow all of Unicode, not just the BMP.
There was a problem hiding this comment.
Added support for unicode characters + test case for that (Ą1 -> Ą2)
| if (v.getVariableType() != null && !v.getSimpleName().equals(v.getVariableType().getName())) { | ||
| return v.withVariableType(v.getVariableType().withName(v.getSimpleName())); | ||
| } |
There was a problem hiding this comment.
Would we be replacing any variable here? Or only when it matched the old variable name? Want to make sure there's no unexpected changes, as the recipe is used so often, and the tests are very minimal.
There was a problem hiding this comment.
This piece of code only updates variable type name if the underlying Identifier has been renamed earlier.
I've added a test case doNotRenameRandomVariablesMatchingClassName to verify that random variables with matching name are not renamed.
There was a problem hiding this comment.
Great thanks! I'm rushing out the door here, but will ask the wider team to review while I'm out over the next few days.
| @Test | ||
| void updateMethodTypeWithUnicodeCharacter() { | ||
| rewriteRun( | ||
| spec -> spec.recipe(new ChangeType("a.Ą1", "a.Ą2", false)), |
There was a problem hiding this comment.
Since Ą is in the BMP, this test should pass with the old code as well. In order to really see the change going from characters to codepoints, you probably need something like 𐐓 (\uD801\uDC13).
There was a problem hiding this comment.
I tried changing Ą/ą to 𐐅/𐐭 but I encountered strange issues from unrelated parts of the code (like import statement started disappearing). I've spent some time debugging it but eventually gave up as it seems to be a broader issue and fixing it is out of scope of this PR.
I reverted the code of decapitalize method to the previous version and removed the test case with special characters. All of my experiments are in the git history in case someone would like to continue the fight from there 😄
What's your motivation?
When I rename a type in IntelliJ IDEA (for example
SomeTypetoAnotherType) it will automatically rename variables with the same name as the class, but starting with lowercase character (sosomeTypetoanotherType).This PR introduces the above feature to
org.openrewrite.java.ChangeTyperecipe.Checklist