Skip to content
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

LocalVariableName: allowOneCharVarInForLoop should allow one char variable in loop #6489

Closed
jack870131 opened this issue Feb 27, 2019 · 9 comments

Comments

Projects
None yet
5 participants
@jack870131
Copy link
Contributor

commented Feb 27, 2019

See comment in #6434 (review)

https://checkstyle.org/config_naming.html#LocalVariableName

Attention: test case is updated, see comments of issue.

Jack@Jack MINGW64 ~/Documents/tmp (new-user)
$ cat TestLocalVariableName.java
class MyClass {
  void MyMethod() {
    for (int var = 1; var < 10; var++) {} // OK
    for (int VAR = 1; VAR < 10; VAR++) {} // violation, name 'VAR' must match
                                          // pattern '^[a-z][a-zA-Z0-9]*$'
    for (int i = 1; i < 10; i++) {} // OK
  }
}
Jack@Jack MINGW64 ~/Documents/tmp (new-user)
$ cat config.xml
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
        <module name="LocalVariableName">
      <property name="severity" value="warning"/>
    </module>
  </module>
</module>
Jack@Jack MINGW64 ~/Documents/tmp (new-user)
$ java $RUN_LOCALE -jar checkstyle-8.18-all.jar -c config.xml TestLocalVariableName.java
Starting audit...
[WARN] C:\Users\asus\Documents\tmp\TestLocalVariableName.java:4:14: Name 'VAR' must match pattern '^[a-z][a-zA-Z0-9]*$'. [LocalVariableName]
Audit done.

Expected:
violation at i variable, as allowOneCharVarInForLoop if false by default.

@romani romani added the approved label Apr 17, 2019

@romani romani changed the title Update allowOneCharVarInForLoop in LocalVariableName Check LocalVariableName in default configuration miss one char variable in loop Apr 17, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Apr 17, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Apr 17, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Apr 17, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Apr 18, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Apr 18, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Apr 18, 2019

@pbludov

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Expected: violation at i variable, as allowOneCharVarInForLoop if false by default.

No. There shouldn't be violation, since i matches pattern ^[a-z][a-zA-Z0-9]*$.

If a team coding style demands at least two characters for the name of a local variable, the pattern should be ^[a-z][a-zA-Z0-9]+$. For at leas three ^[a-z][a-zA-Z0-9]{2,}$ and so on.

The property allowOneCharVarInForLoop is irrelevant there. But it can be used to make an exception.
For example:

<module name="LocalVariableName">
  <property name="allowOneCharVarInForLoop" value="true"/>
  <property name="format" value="^[a-z][a-zA-Z0-9]{2,}$"/>
</module>

and

class MyClass {
  void MyMethod() {
    for (int i = 1; i < 10; i++) {
      int j = i;
    }
  }
}

Gives

Starting audit...
[WARN] /home/pbludov/src/tmp/TestClass.java:4:29: Name 'j' must match pattern '^[a-z][a-zA-Z0-9]{2,}$'. [LocalVariableName]
Audit done.
@rnveach

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@romani Please explain approval and why allow property should cause a violation if it is turned off and regexp still passes.
This property was added as part of google at #192 for http://checkstyle.sourceforge.net/styleguides/google-java-style-20170228.html#s5.2.7-local-variable-names .

If a team coding style demands at least two characters for the name of a local variable

Original post doesn't make it sound like they want 2 characters for all local variables. They just want it specifically for all loop variables and there isn't a way to specify it without affecting the other as it isn't customizable.
Example:

for (int hh = 1; hh < 10; hh++) {} // OK
for (int i = 1; i < 10; i++) {} // wants violation
int j; // OK
int kk; // OK
@romani

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

No. There shouldn't be violation, since i matches pattern ^[a-z][a-zA-Z0-9]*$.

Good catch, I confirm, there should not be violation.

But it can be used to make an exception. For example:

yes, There should not be violation. @strkkk , this is a target of this issue.

Please explain approval and why allow property should cause a violation if it is turned off and regexp still passes

if allowOneCharVarInForLoop=true and regexp is passing, should be no violation.
BUT case from @pbludov, should not make violation as allowOneCharVarInForLoop=true is extra allowance after regexp validation.


@strkkk , please update documentation example https://checkstyle.org/config_naming.html#LocalVariableName for An example of one character variable name in initialization expression(like "i") in FOR loop to define non default regexp(for example lets demand 3 symbols minimum for name) to make allowOneCharVarInForLoop effective. In example should be clear that names like i are allowed but 'ii' are not.

@romani romani removed their assignment Apr 27, 2019

@strkkk

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

Just to summarize things, this should work like:
allowProp_Regex matches_Result
true______true_________ no violation
true______false ________no volation
false_____true__________no violation
false_____false_________violation

So, case from original issue is not a bug.

Things to correct:

  1. Docs
  2. Tests and code

@romani
There is also a question about doc correction. Regex for check is loop variable(^a-z$) has one char or not is actually checking is there one lower-case char, but upper-case char is not prohibited explicitly.
Should regex be corrected or documentation?

@strkkk

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@romani In example from @pbludov violation is raised inside loop, not in for initialization statement.

As I see, the target of this property is to make an exception to for initialization statement only, not to whole for loop body.

Please confirm - if this property should be extended to whole for loop body or not?
If not, there is no problems in the code, only documentation can be clearer.

@romani

This comment has been minimized.

Copy link
Member

commented May 5, 2019

but upper-case char is not prohibited explicitly. Should regex be corrected or documentation?

Uhh, such a weird code:
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/naming/LocalVariableNameCheck.java#L113
it was added during 32ca737 , I am not sure why we have such code, I even did not put any comment on PR before merge - #193 , no comments in issue, that is really strange.

I see no reason in regexp for single char. I think it is simply check length of name is enough. User should be able to use even all-uppercased code if it is reasonable in his context.

so it should be:

RegExpMatches_Result allowXXX_Result Violation?
true true no
true false no
false true no
false false violation

if this property should be extended to whole for loop body or not?

No.
On code:

for (int i = 1; i < 10; i++) {
      int j = i;
    }

option allowOneCharVarInForLoop=true, should affect only violation on i. j should follow matching of regexp.

strkkk added a commit to strkkk/checkstyle that referenced this issue May 5, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 5, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 14, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 14, 2019

rnveach added a commit that referenced this issue May 23, 2019

@rnveach rnveach added the bug label May 23, 2019

@rnveach rnveach added this to the 8.21 milestone May 23, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Fix was merged

@rnveach rnveach closed this May 23, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@romani Should issue title be updated for release notes?

@romani romani changed the title LocalVariableName in default configuration miss one char variable in loop LocalVariableName: allowOneCharVarInForLoop should allow one char variable in loop May 26, 2019

@romani

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Updated, you can update it further to make more clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.