RightCurly was misconfigured in google_checks.xml for do-while blocks #3678

Closed
romani opened this Issue Dec 29, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Dec 29, 2016

Regression in RightCurly from 6.19 to 7.0 , still present in 7.4.
in do-while blocks, checkstyle demand while to be on new line. But this is against Google's style - https://google.github.io/styleguide/javaguide.html#s4.1.2-blocks-k-r-style , Line break after the closing brace, only if that brace terminates a statement or terminates the body of a method, constructor, or named class. For example, there is no line break after the brace if it is followed by ELSE or a comma.

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
                        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="charset" value="UTF-8"/>
  <property name="severity" value="warning"/>
  <module name="TreeWalker">
    <module name="RightCurly">
        <property name="id" value="RightCurlyAlone"/>
        <property name="option" value="alone"/>
        <property name="tokens" value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE, LITERAL_DO, STATIC_INIT, INSTANCE_INIT"/>
    </module>
  </module>
</module>

$ cat MyClass.java 
public final class MyClass {
    public void foo(String publicPar){
      do {
        // do smth
      } while (publicPar != null);
    }
}

$ java -jar checkstyle-7.4-SNAPSHOT-all.jar -c config.xml MyClass.java 
Starting audit...
[WARN] /var/tmp/MyClass.java:5:7: '}' at column 7 should be alone on a line. [RightCurly]
Audit done.
rivanov@p5510:/var/tmp$ java -jar checkstyle-6.19-all.jar -c config.xml MyClass.java 
Starting audit...
Audit done.
rivanov@p5510:/var/tmp$ java -jar checkstyle-7.0-all.jar -c config.xml MyClass.java 
Starting audit...
[WARN] /var/tmp/MyClass.java:5:7: '}' at column 7 should be alone on a line. [RightCurly]
Audit done.

http://checkstyle.sourceforge.net/releasenotes.html#Release_7.0
caused by: #3090

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 29, 2016

Member

to avoid violation, LITERAL_DO, should be removed from tokens.

Member

romani commented Dec 29, 2016

to avoid violation, LITERAL_DO, should be removed from tokens.

@romani romani self-assigned this Dec 29, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 29, 2016

Member

Reason of the issue is that not all our ITs for google style use actual google config.
Files that use custom configs:

~/java/github/checkstyle/checkstyle/src/it/java [i3678-RightCurly L|✔] 
$ grep -r "createCheckConfig" . --exclude="BaseCheckTestSupport.java"
./com/google/checkstyle/test/chapter4formatting/rule451wheretobreak/OperatorWrapTest.java:        final DefaultConfiguration newCheckConfig = createCheckConfig(OperatorWrapCheck.class);
./com/google/checkstyle/test/chapter4formatting/rule412nonemptyblocks/RightCurlyTest.java:        final DefaultConfiguration newCheckConfig = createCheckConfig(RightCurlyCheck.class);
./com/google/checkstyle/test/chapter4formatting/rule412nonemptyblocks/RightCurlyTest.java:        final DefaultConfiguration newCheckConfig = createCheckConfig(RightCurlyCheck.class);
./com/google/checkstyle/test/chapter4formatting/rule412nonemptyblocks/RightCurlyTest.java:        final DefaultConfiguration checkConfig = createCheckConfig(RightCurlyCheck.class);
./com/google/checkstyle/test/chapter2filebasic/rule231filetab/FileTabCharacterTest.java:            createCheckConfig(FileTabCharacterCheck.class);
./com/google/checkstyle/test/chapter7javadoc/rule711generalform/SingleLineJavadocTest.java:        final DefaultConfiguration checkConfig = createCheckConfig(SingleLineJavadocCheck.class);

Expected approach for testing is usage of getCheckConfig:

final Configuration checkConfig = getCheckConfig("LeftCurly");
final String filePath = getPath("InputLeftCurlyMethod.java");
Member

romani commented Dec 29, 2016

Reason of the issue is that not all our ITs for google style use actual google config.
Files that use custom configs:

~/java/github/checkstyle/checkstyle/src/it/java [i3678-RightCurly L|✔] 
$ grep -r "createCheckConfig" . --exclude="BaseCheckTestSupport.java"
./com/google/checkstyle/test/chapter4formatting/rule451wheretobreak/OperatorWrapTest.java:        final DefaultConfiguration newCheckConfig = createCheckConfig(OperatorWrapCheck.class);
./com/google/checkstyle/test/chapter4formatting/rule412nonemptyblocks/RightCurlyTest.java:        final DefaultConfiguration newCheckConfig = createCheckConfig(RightCurlyCheck.class);
./com/google/checkstyle/test/chapter4formatting/rule412nonemptyblocks/RightCurlyTest.java:        final DefaultConfiguration newCheckConfig = createCheckConfig(RightCurlyCheck.class);
./com/google/checkstyle/test/chapter4formatting/rule412nonemptyblocks/RightCurlyTest.java:        final DefaultConfiguration checkConfig = createCheckConfig(RightCurlyCheck.class);
./com/google/checkstyle/test/chapter2filebasic/rule231filetab/FileTabCharacterTest.java:            createCheckConfig(FileTabCharacterCheck.class);
./com/google/checkstyle/test/chapter7javadoc/rule711generalform/SingleLineJavadocTest.java:        final DefaultConfiguration checkConfig = createCheckConfig(SingleLineJavadocCheck.class);

Expected approach for testing is usage of getCheckConfig:

final Configuration checkConfig = getCheckConfig("LeftCurly");
final String filePath = getPath("InputLeftCurlyMethod.java");
@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 29, 2016

Contributor

@romani
See our discussion at #3321 . Why is it considered as wrong behavior now? Alone means alone. Violation message for SAME brace policy: ... as the next part of ... statement. Did you mean that our ITs are broken?

Contributor

MEZk commented Dec 29, 2016

@romani
See our discussion at #3321 . Why is it considered as wrong behavior now? Alone means alone. Violation message for SAME brace policy: ... as the next part of ... statement. Did you mean that our ITs are broken?

romani added a commit that referenced this issue Dec 30, 2016

Issue #3678: RightCurly with DO-WHILE demands WHILE to be on new line…
… (configuration problem with RightCurly check in google_checks.xml). Update for ITs.

romani added a commit that referenced this issue Dec 30, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 30, 2016

Member

Did you mean that our ITs are broken?

@MEZk , yes. Check is fine. Few of our ITs used non google_checks.xml confutations.
Google config had problem too, from 7.0 till 7.3, I we forgot to update it.
Please do code review for my update.

Member

romani commented Dec 30, 2016

Did you mean that our ITs are broken?

@MEZk , yes. Check is fine. Few of our ITs used non google_checks.xml confutations.
Google config had problem too, from 7.0 till 7.3, I we forgot to update it.
Please do code review for my update.

@romani romani referenced this issue in pgjdbc/pgjdbc Dec 30, 2016

Merged

resolve violations from checkstyle 7.4 #725

romani added a commit that referenced this issue Dec 30, 2016

Issue #3678: RightCurly with DO-WHILE demands WHILE to be on new line…
… (configuration problem with RightCurly check in google_checks.xml). Update for ITs.

romani added a commit that referenced this issue Dec 30, 2016

@romani romani changed the title from RightCurly with DO-WHILE demands WHILE to be on new line to RightCurly was misconfigured in google_checks.xml for do-while blocks Dec 30, 2016

@romani romani added the bug label Dec 30, 2016

@romani romani added this to the 7.4 milestone Dec 30, 2016

@romani romani closed this Dec 30, 2016

@mishako mishako referenced this issue in spotify/spotify-checkstyle-config Feb 28, 2017

Merged

Sync right-curly rules with google #17

@ssoloff ssoloff referenced this issue in triplea-game/triplea Apr 11, 2017

Merged

Checkstyle: Fix token placement violations #1635

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment