Skip to content

RenameLocalVariablesToCamelCase: Do not rename method argument names. #205

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.

Sign up for GitHub

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

Merged
pstreef merged 2 commits into from
Oct 23, 2023
Merged

RenameLocalVariablesToCamelCase: Do not rename method argument names. #205

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d57926d
Fix: Many frameworks depend on method parameter names to use for requ…
pstreef Oct 22, 2023
89ee276
remove annotations
pstreef Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/main/java/org/openrewrite/staticanalysis/RenameLocalVariablesToCamelCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public String getDescription() {
return "Reformat local variable and method parameter names to camelCase to comply with Java naming convention. " +
"The recipe will not rename variables declared in for loop controls or catches with a single character. " +
"The first character is set to lower case and existing capital letters are preserved. " +
"Special characters that are allowed in java field names `$` and `_` are removed. " +
"Special characters that are allowed in java field names `$` and `_` are removed (unless the name starts with one). " +
"If a special character is removed the next valid alphanumeric will be capitalized. " +
"Currently, does not support renaming members of classes. " +
"The recipe will not rename a variable if the result already exists in the class, conflicts with a java reserved keyword, or the result is blank.";
Expand Down Expand Up @@ -105,7 +105,7 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m

private boolean isLocalVariable(J.VariableDeclarations mv) {
// The recipe will not rename variables declared in for loop controls or catches.
if (!isInMethodDeclarationBody() || isDeclaredInForLoopControl() || isDeclaredInCatch()) {
if (!isInMethodDeclarationBody() || isDeclaredInForLoopControl() || isDeclaredInCatch() || isMethodArgument()) {
return false;
}

Expand All @@ -119,6 +119,11 @@ private boolean isLocalVariable(J.VariableDeclarations mv) {
return true;
}

private boolean isMethodArgument() {
return getCursor().getParentTreeCursor()
.getValue() instanceof J.MethodDeclaration;
}

private boolean isInMethodDeclarationBody() {
return getCursor().dropParentUntil(p -> p instanceof J.MethodDeclaration ||
p instanceof J.ClassDeclaration ||
Expand Down
30 changes: 26 additions & 4 deletions src/test/java/org/openrewrite/staticanalysis/RenameLocalVariablesToCamelCaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
*/
package org.openrewrite.staticanalysis;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junitpioneer.jupiter.ExpectedToFail;
import org.openrewrite.DocumentExample;
import org.openrewrite.Issue;
import org.openrewrite.Recipe;
Expand Down Expand Up @@ -89,23 +91,23 @@ void renameLocalVariables() {
class Test {
int DoNoTChange;

public int addTen(int rename_one) {
public int addTen(int dont_rename_one) {
double RenameTwo = 2.0;
float __rename__three__ = 2.0;
long _Rename__Four = 2.0;
return rename_one + RenameTwo + __rename__three__ + _Rename__Four + 10;
return dont_rename_one + RenameTwo + __rename__three__ + _Rename__Four + 10;
}
}
""",
"""
class Test {
int DoNoTChange;

public int addTen(int renameOne) {
public int addTen(int dont_rename_one) {
double renameTwo = 2.0;
float renameThree = 2.0;
long renameFour = 2.0;
return renameOne + renameTwo + renameThree + renameFour + 10;
return dont_rename_one + renameTwo + renameThree + renameFour + 10;
}
}
"""
Expand All @@ -114,6 +116,7 @@ public int addTen(int renameOne) {
}

@SuppressWarnings("JavadocDeclaration")
@Disabled
Copy link
Contributor Author

@pstreef pstreef Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@joanvr joanvr Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could try to rename param references in annotations too? Although I'm not sure if we can have a generic enough approach

Copy link
Contributor Author

@pstreef pstreef Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Issue("https://github.com/openrewrite/rewrite/issues/2437")
@Test
void renameJavaDocParam() {
Expand Down Expand Up @@ -438,6 +441,7 @@ void test() {

@Test
void renameFinalLocalVariables() {
//language=java
rewriteRun(
java(
"""
Expand All @@ -458,4 +462,22 @@ void test() {
);
}

@Test
void doNotRenameMethodArguments(){
//language=java
rewriteRun(
java(
"""
@Controller
Copy link
Contributor

@joanvr joanvr Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those anotations needed for the test? They are not imported anyway, thus giving incomplete type attribution... maybe we could remove them for simplicity.

Copy link
Contributor Author

@pstreef pstreef Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@knutwannheden knutwannheden Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Soon we will flag these missing type attributions in before sources as errors (unless the before type validation is configured to ignore these).

Copy link
Contributor

@joanvr joanvr Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

@pstreef pstreef Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

class MyController {
@GetMapping
String getHello(@RequestParam String your_name) {
return "hello " + your_name;
}
}
"""
)
);
}

}