-
Notifications
You must be signed in to change notification settings - Fork 39
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
Introduce ConstantNaming
check
#794
Introduce ConstantNaming
check
#794
Conversation
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
In addition to class variables within the same compilation unit, should we also match on constants imported from other classes ?
Ideally, the checker will suggest the fix in that class 🤔
Which raises the other question, do we really need to suggest a fix for the other classes referencing the constants in this
compilation unit ?
@Stephan202 suggested the use of TreeScanner
as in this example but I could not seem to make this work 😓
Help appreciated 🙏
31a9e4d
to
7fff49f
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Tnx for working on this @mohamedsamehsalah; it'll allow for some nice cleanup I expect. Did leave some comments based on a quick skim :)
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
Outdated
Show resolved
Hide resolved
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
Outdated
Show resolved
Hide resolved
...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java
Outdated
Show resolved
Hide resolved
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
Outdated
Show resolved
Hide resolved
(I didn't check the comments before reviewing; sorry.)
Error Prone doesn't support this; for cases like that OpenRewrite would be better suited. (But for now, let's just focus on the private fields.)
Anywhere we can see the code that doesn't work? Perhaps I can take a look. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
4acc2bc
to
ecdccc3
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
ecdccc3
to
40a2a6f
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
@Stephan202 I covered your first pass comments, however, I still could not make use of the AFAIU, the suggested fix builder used in this PR will execute what you are suggesting in this comment:
Happy to hear your thoughts (again? 😅) 🙏 |
40a2a6f
to
0c94940
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
@Stephan202 I covered your first pass comments, however, I still could not make use of the
TreeScanner
. Or in other words, I couldn't find what advantage does theTreeScanner
give that theTreeMatcher
does not.
Actually, you are using a TreeScanner
now, just through a method I didn't know existed: have a close look at SuggestedFixes.renameVariable
👀 😄 This should work!
Before I dive into a more thorough review: would you like to have a crack at resolving the surviving mutants?
if (!isVariableUpperSnakeCase(variableName) && !isVariableNameExcluded(variableName)) { | ||
String replacement = toUpperSnakeCase(variableName); | ||
|
||
if (!classVariables.contains(replacement)) { |
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.
IIUC, here we avoid renaming a variable if the target name is already declared in the exact same scope. Due to the global search and replace that we do, I suspect that any usage of the target name in this CompilationUnit
could cause ambiguity, shadowing or confusion. So for this case, too, a TreeScanner
may be employed.
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.
would you like to have a crack at resolving the surviving mutants?
Yes, sir 🖖
IIUC, here we avoid renaming a variable if the target name is already declared in the exact same scope. Due to the global search and replace that we do, I suspect that any usage of the target name in this CompilationUnit could cause ambiguity, shadowing or confusion. So for this case, too, a TreeScanner may be employed.
Will look into this 👀
Thanks.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1c55636
to
12e51ab
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Kudos, SonarCloud Quality Gate passed! |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Sorry for the slow progress on this @Stephan202 , PTAL 🙏
12e51ab
to
a7f652e
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
@Inject | ||
ConstantNaming(ErrorProneFlags flags) { | ||
exemptedNames = | ||
Sets.union(DEFAULT_EXEMPTED_NAMES, Flags.getSet(flags, ADDITIONAL_EXEMPTED_NAMES_FLAG)) |
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.
Here and below: Pitest correctly flags that the Sets#union
arguments can be swapped without causing a test failure. That's expected, as set union is a symmetric operation, and this code doesn't care about the created set's iteration order.
94fb2a4
to
d7a3938
Compare
Quality Gate passedIssues Measures |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
new TreeScanner<Boolean, @Nullable Void>() { | ||
@Override | ||
public Boolean visitVariable(VariableTree tree, @Nullable Void unused) { | ||
return ASTHelpers.getSymbol(tree).getSimpleName().toString().equals(name) |
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.
I'm not sure what your style guide is, but fwiw Google's is popular and does not follow this rule (e.g. https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names |
Thanks for sharing @ben-manes! I suppose this one of the (likely few) ways in which inside Picnic we deviate from the Google style guide. In version 0.19.0 we also landed #783, which by default will suggest |
Suggested commit message: