RightCurly: update documentation for singleline statements with SAME option #4085

Closed
Vladlis opened this Issue Mar 22, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@Vladlis
Member

Vladlis commented Mar 22, 2017

$ cat TestClass.java
public class TestClass {
    void method() {
        if (true) { getClass(); int i; }
    }
}

$ cat TestConfig.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"/>

    <module name="TreeWalker">
         <module name="RightCurly">
             <property name="option" value="same"/>
         </module>
    </module>
</module>

$ java -jar checkstyle-7.6-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

Expected: violation on IF
This issue is reproducible with the other multiblock statements as well.

If this behaviour is by design, then documentation should be extended.
http://checkstyle.sourceforge.net/config_blocks.html#RightCurly

@Vladlis Vladlis changed the title from RightCurly: false negative on IF with SAME option to RightCurly: false negative on multiblock statements with SAME option Mar 22, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 23, 2017

Member

from documentation http://checkstyle.sourceforge.net/property_types.html#rcurly

    // Single line blocks will rise violations, because right curly
    // brace is not on the same line as the next part of a multi-block
    // statement, it just ends the line.
    public long getId() {return id;} // this is NOT OK

so violation is expected or not ????
Examples need to be extended to have single line cases like:

if (true) {doSmth();}
if (true) {doSmth();} esle {doSmthElse();}

rationale is too keep behavior consistent with/without esle {doSmthElse();}

$ 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"/>
    <module name="TreeWalker">
         <module name="RightCurly">
             <property name="option" value="same"/>
         </module>
    </module>
</module>

$ cat Test.java 
public class Test {
    public void demo() {
        if (true) {doSmth();}
        if (true) {doSmth();} else {doSmthElse();}
        if (true) {
            doSmth();
        }
        if (true) {
            doSmth();     } ///violation
        doSmth();
        if (true) {
            doSmth();
        } else {
            doSmthElse();
        }
    }

    void doSmth() {}
    void doSmthElse() {}
}

$ java -jar checkstyle-7.6-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] Test.java:9:27: '}' at column 27 should have line break before. [RightCurly]
Audit done.
Checkstyle ends with 1 errors.

behavior is correct to what we have now as logic in this Check.
So same is ignoring singleline statements.
In other words for non-multi-blocks same works the same as alone_or_sinlgeline mode. (this conclusion is reasonable to put to xdoc)

There is no way in Check to enforce "}" in same mode to be on new line..... sounds like new mode for a Check

Member

romani commented Mar 23, 2017

from documentation http://checkstyle.sourceforge.net/property_types.html#rcurly

    // Single line blocks will rise violations, because right curly
    // brace is not on the same line as the next part of a multi-block
    // statement, it just ends the line.
    public long getId() {return id;} // this is NOT OK

so violation is expected or not ????
Examples need to be extended to have single line cases like:

if (true) {doSmth();}
if (true) {doSmth();} esle {doSmthElse();}

rationale is too keep behavior consistent with/without esle {doSmthElse();}

$ 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"/>
    <module name="TreeWalker">
         <module name="RightCurly">
             <property name="option" value="same"/>
         </module>
    </module>
</module>

$ cat Test.java 
public class Test {
    public void demo() {
        if (true) {doSmth();}
        if (true) {doSmth();} else {doSmthElse();}
        if (true) {
            doSmth();
        }
        if (true) {
            doSmth();     } ///violation
        doSmth();
        if (true) {
            doSmth();
        } else {
            doSmthElse();
        }
    }

    void doSmth() {}
    void doSmthElse() {}
}

$ java -jar checkstyle-7.6-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] Test.java:9:27: '}' at column 27 should have line break before. [RightCurly]
Audit done.
Checkstyle ends with 1 errors.

behavior is correct to what we have now as logic in this Check.
So same is ignoring singleline statements.
In other words for non-multi-blocks same works the same as alone_or_sinlgeline mode. (this conclusion is reasonable to put to xdoc)

There is no way in Check to enforce "}" in same mode to be on new line..... sounds like new mode for a Check

@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis Mar 23, 2017

Member

@romani
I've found the reason of such behaviour: #1416
I guess documentation should be extended, as this is not obvious.

Member

Vladlis commented Mar 23, 2017

@romani
I've found the reason of such behaviour: #1416
I guess documentation should be extended, as this is not obvious.

@romani romani changed the title from RightCurly: false negative on multiblock statements with SAME option to RightCurly: update documentation for singleline statements with SAME option Mar 23, 2017

@romani romani added the approved label Mar 23, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 23, 2017

Member

issue subject is changed , documentation is expected to be updated.

Member

romani commented Mar 23, 2017

issue subject is changed , documentation is expected to be updated.

@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis Mar 25, 2017

Member

I am on it

Member

Vladlis commented Mar 25, 2017

I am on it

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 5, 2017

Member

Fix is merged

Member

rnveach commented Apr 5, 2017

Fix is merged

@rnveach rnveach closed this Apr 5, 2017

@rnveach rnveach added this to the 7.7 milestone Apr 5, 2017

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