Issue #14688 : LocalVaiableNameCheck should allow unnamed variables b…#14978
Conversation
|
Github, generate report |
|
this check will break on legacy code that can use |
8dc9e6d to
9a563ce
Compare
| result = !isFinal && ScopeUtil.isLocalVariableDef(ast); | ||
| } | ||
| return result; | ||
| return result && !TokenUtil.isUnnamed(ast.findFirstToken(TokenTypes.IDENT)); |
There was a problem hiding this comment.
There are 2 ways to handle this issue. Either we update the existing property regexp to allow underscores, or we update the code to fully ignore single underscore variable names.
The regexp allows anyone to change it and what it accepts. Changing it would allow the users to backward compatible this change.
Change the code hard codes that this is to be a full on ignore which can't be undone. This is similar to ignore final variables, as another check handles this. If someone wanted to configure CS to violate underscore variables again, they would have to use another check, IllegalTokenText or possibly IllegalIdentifierName.
Are we sure we want to basically break compatibility and hard code the ignore? I was originally thinking we would just update the property. I am leaning more to the property just to remain compatible with those not on 21 yet. I remember somewhere we said we wanted to keep backwards compatibility with another check.
@nrmancuso Feel free to review.
There was a problem hiding this comment.
I was originally thinking we would just update the property.
Yes, please do this @mahfouz72
e13a5fd to
5692fca
Compare
…riables by default
5692fca to
d3e56f0
Compare
|
Github, generate site |
|
Github, generate report |
|
Report is a bit hard to consume, but looks good when you click into projects with an asymmetrical diff and find the places where unnamed variables are present, you can see that there violations have been removed. |
nrmancuso
left a comment
There was a problem hiding this comment.
Approved, but please make a comment in the issue with the old regexp to help people to find it and restore this check's previous behavior.
|
Since we will probably have more regexp property updates, change the message of the check to a static message so we can see new/removed violations. |
|
Github, generate report |
closes #14688 :
Diff Regression config: https://gist.githubusercontent.com/mahfouz72/afd57aa6e51ca161b7927acd959e5211/raw/ec67844f3c91caaff9c853fa121a42c69ec2e0df/check.xml
Diff Regression projects: https://gist.githubusercontent.com/mahfouz72/a3d0af030c8f5efd0d8a39f2c14750bc/raw/a3424ad9b6f722e5de1a927e3d85e383101b2b93/projects-to-test-on.properties