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

EmptyLineSeparator check not enforcing empty line after class def, enum def or interface def tokens #3089

Closed
embee1981 opened this Issue Apr 10, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@embee1981

embee1981 commented Apr 10, 2016

Using checkstyle 6.17, I'm trying to enforce that code should be formatted with a single whitespace after the class, enum or interface definition eg.

public class Sample {

    private String test;
}

I believe the EmptyLineSeparator should be enforcing this for me, code and config sample below :-)

E:\>javac *.java

E:\>cat Sample.java
public class Sample { //EmptyLineSeparator should check for empty line after CLASS_DEF token
    private String test;
}

E:\>cat SampleEnum.java
public enum SampleEnum { //EmptyLineSeparator should check for empty line after ENUM_DEF token
    HELLO, WORLD;
}

E:\>cat ISample.java
public interface ISample { //EmptyLineSeparator should check for empty line after INTERFACE_DEF token
    void blah();
}

E:\>cat checkstyle.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">
    <module name="TreeWalker">
        <module name="EmptyLineSeparator">
            <property name="tokens" value="CLASS_DEF, INTERFACE_DEF, ENUM_DEF" />
            <property name="allowMultipleEmptyLines" value="false" />
        </module>
    </module>
</module>

E:\>java -jar checkstyle-6.17-all.jar -c checkstyle.xml *.java
Starting audit...
Audit done.

Expectation:

Starting audit...
[ERROR] E:\Sample.java:2: 'VARIABLE_DEF' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] E:\SampleEnum.java:2: 'VARIABLE_DEF' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] E:\ISample.java:2: 'VARIABLE_DEF' should be separated from previous statement. [EmptyLineSeparator]
Audit done.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 10, 2016

Member

http://checkstyle.sourceforge.net/config_whitespace.html#EmptyLineSeparator

Checks for empty line separators after header, package, all import declarations, fields, constructors, methods, nested classes, static initializers and instance initializers.

you request looks like reasonable.
Examples at xdoc files need to be updated.

behaviour is strange :

/var/tmp $ cat -n TestClass.java 
     1  ///////////////////////////////////////////////////
     2  //HEADER
     3  ///////////////////////////////////////////////////
     4  package com.puppycrawl.tools.checkstyle.whitespace;
     5  import java.io.Serializable;
     6  class Foo {
     7      public static final int FOO_CONST = 1;
     8      public void foo() {}
     9  }
/var/tmp $ java -jar checkstyle-6.17-SNAPSHOT-all.jar -c Test.xml TestClass.java 
Starting audit...
[ERROR] /var/tmp/TestClass.java:4: 'package' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] /var/tmp/TestClass.java:5: 'import' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] /var/tmp/TestClass.java:6: 'CLASS_DEF' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] /var/tmp/TestClass.java:8: 'METHOD_DEF' should be separated from previous statement. [EmptyLineSeparator]
Audit done.
Checkstyle ends with 4 errors.

No violation on line 7 (before field declaration), but it is expected.

Member

romani commented Apr 10, 2016

http://checkstyle.sourceforge.net/config_whitespace.html#EmptyLineSeparator

Checks for empty line separators after header, package, all import declarations, fields, constructors, methods, nested classes, static initializers and instance initializers.

you request looks like reasonable.
Examples at xdoc files need to be updated.

behaviour is strange :

/var/tmp $ cat -n TestClass.java 
     1  ///////////////////////////////////////////////////
     2  //HEADER
     3  ///////////////////////////////////////////////////
     4  package com.puppycrawl.tools.checkstyle.whitespace;
     5  import java.io.Serializable;
     6  class Foo {
     7      public static final int FOO_CONST = 1;
     8      public void foo() {}
     9  }
/var/tmp $ java -jar checkstyle-6.17-SNAPSHOT-all.jar -c Test.xml TestClass.java 
Starting audit...
[ERROR] /var/tmp/TestClass.java:4: 'package' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] /var/tmp/TestClass.java:5: 'import' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] /var/tmp/TestClass.java:6: 'CLASS_DEF' should be separated from previous statement. [EmptyLineSeparator]
[ERROR] /var/tmp/TestClass.java:8: 'METHOD_DEF' should be separated from previous statement. [EmptyLineSeparator]
Audit done.
Checkstyle ends with 4 errors.

No violation on line 7 (before field declaration), but it is expected.

@romani romani added the approved label May 3, 2016

@robertpainsi

This comment has been minimized.

Show comment
Hide comment
@robertpainsi

robertpainsi Feb 7, 2017

Contributor

I like the idea! I still think we need to discuss the specification.

Currently, the EmptyLineSeparator works based on siblings.

// tokens="CLASS_DEF"
1: class Foo { // has no sibling
2:     class Bar { // sibling is int i = 0;
3:     }
4:     int i = 0; // violation, sibling is class Bar {}
5: }

As you can see, siblings have the same indentation level.

Now to the discussion:

// tokens="CLASS_DEF"
1: class Foo {
2:     class Bar {}
3:     class Qux {} // violation
4: }

Should there be a violation on line 2 and 3 with just using the token CLASS_DEF?
If yes, you can't fine tune the violations. I will use ENUM_DEF as an example.

// tokens="ENUM_DEF"
1: class Foo {
2:
3:     enum Bar {
4:         Qux
5:     }
6:     int i = 0; // violation
7: }

The enum on line 3,4,5 definitely looks fine, line 6 does not (missing newline). However I can't use this check if I want to allow such enum styles but also want to separate the enums (right curly brace) from the next statement (int i = 0;).

I also think it would be useful to have a NoEmptyLineSeparator. E.g. the enum example above and also:

1: class Foo { // no empty line after {
2:     private static final String TAG = "Foo";
3:
4:     public void bar() { // no empty line after {
5:         qux();
6:     }
7: }

Especially after the curly brace of a method. I'm just mentioning it because it may have an influence to the specification.

Contributor

robertpainsi commented Feb 7, 2017

I like the idea! I still think we need to discuss the specification.

Currently, the EmptyLineSeparator works based on siblings.

// tokens="CLASS_DEF"
1: class Foo { // has no sibling
2:     class Bar { // sibling is int i = 0;
3:     }
4:     int i = 0; // violation, sibling is class Bar {}
5: }

As you can see, siblings have the same indentation level.

Now to the discussion:

// tokens="CLASS_DEF"
1: class Foo {
2:     class Bar {}
3:     class Qux {} // violation
4: }

Should there be a violation on line 2 and 3 with just using the token CLASS_DEF?
If yes, you can't fine tune the violations. I will use ENUM_DEF as an example.

// tokens="ENUM_DEF"
1: class Foo {
2:
3:     enum Bar {
4:         Qux
5:     }
6:     int i = 0; // violation
7: }

The enum on line 3,4,5 definitely looks fine, line 6 does not (missing newline). However I can't use this check if I want to allow such enum styles but also want to separate the enums (right curly brace) from the next statement (int i = 0;).

I also think it would be useful to have a NoEmptyLineSeparator. E.g. the enum example above and also:

1: class Foo { // no empty line after {
2:     private static final String TAG = "Foo";
3:
4:     public void bar() { // no empty line after {
5:         qux();
6:     }
7: }

Especially after the curly brace of a method. I'm just mentioning it because it may have an influence to the specification.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 4, 2017

Member

I reread issue and documentation again .....

from html documentation "..... line separators after ....." there is no word about "{" or "}". But usually "}" is signal for end of method/class. So some cases are covered.
Rules for empty line after "{" are a bit complicated there a a lot cases there it is done on purpose and not ....

Member

romani commented Mar 4, 2017

I reread issue and documentation again .....

from html documentation "..... line separators after ....." there is no word about "{" or "}". But usually "}" is signal for end of method/class. So some cases are covered.
Rules for empty line after "{" are a bit complicated there a a lot cases there it is done on purpose and not ....

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 4, 2017

Member

@robertpainsi ,

I updated your examples to show violations on current implementation of Check, to make it clear what is now and what you would like to have.

siblings have the same indentation level

we can not rely on indentation. We should rely on AST structure only. Siblings should be in tree.

Should there be a violation on line 2 and 3 with just using the token CLASS_DEF?

only on line 3, as Check take care about empty line "after" only (according to its description).

line 6 does not (missing newline)

line 6 miss empty line in front of it, that is why there is violation.

However I can't use this check if I want to allow such enum styles but also want to separate the enums (right curly brace) from the next statement (int i = 0;).

it works like you describe, I checked it , violation is only on line 6.

NoEmptyLineSeparator

please create separate issue on this. This check is simple , might be used by somebody.

Member

romani commented Mar 4, 2017

@robertpainsi ,

I updated your examples to show violations on current implementation of Check, to make it clear what is now and what you would like to have.

siblings have the same indentation level

we can not rely on indentation. We should rely on AST structure only. Siblings should be in tree.

Should there be a violation on line 2 and 3 with just using the token CLASS_DEF?

only on line 3, as Check take care about empty line "after" only (according to its description).

line 6 does not (missing newline)

line 6 miss empty line in front of it, that is why there is violation.

However I can't use this check if I want to allow such enum styles but also want to separate the enums (right curly brace) from the next statement (int i = 0;).

it works like you describe, I checked it , violation is only on line 6.

NoEmptyLineSeparator

please create separate issue on this. This check is simple , might be used by somebody.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 5, 2017

Member

unfortunately I need to close this issue.
As Check work as designed and described in documentation.

$ java -jar checkstyle-7.5-all.jar -t ISample.java 
INTERFACE_DEF -> INTERFACE_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_INTERFACE -> interface [1:7]
|--IDENT -> ISample [1:17]
`--OBJBLOCK -> OBJBLOCK [1:25]
    |--LCURLY -> { [1:25]
    |--METHOD_DEF -> METHOD_DEF [2:4]
    |   |--MODIFIERS -> MODIFIERS [2:4]
    |   |--TYPE -> TYPE [2:4]
    |   |   `--LITERAL_VOID -> void [2:4]
    |   |--IDENT -> blah [2:9]
    |   |--LPAREN -> ( [2:13]
    |   |--PARAMETERS -> PARAMETERS [2:14]
    |   |--RPAREN -> ) [2:14]
    |   `--SEMI -> ; [2:15]
    `--RCURLY -> } [3:0]

After "INTERFACE_DEF" there are no other class/interface so no need to do separation.
Check do separation between siblings base on AST tree. There is no sibling for "INTERFACE_DEF".

Request for some functionality to enforce empty line after "{" will be addressed at #3379.

Member

romani commented Mar 5, 2017

unfortunately I need to close this issue.
As Check work as designed and described in documentation.

$ java -jar checkstyle-7.5-all.jar -t ISample.java 
INTERFACE_DEF -> INTERFACE_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_INTERFACE -> interface [1:7]
|--IDENT -> ISample [1:17]
`--OBJBLOCK -> OBJBLOCK [1:25]
    |--LCURLY -> { [1:25]
    |--METHOD_DEF -> METHOD_DEF [2:4]
    |   |--MODIFIERS -> MODIFIERS [2:4]
    |   |--TYPE -> TYPE [2:4]
    |   |   `--LITERAL_VOID -> void [2:4]
    |   |--IDENT -> blah [2:9]
    |   |--LPAREN -> ( [2:13]
    |   |--PARAMETERS -> PARAMETERS [2:14]
    |   |--RPAREN -> ) [2:14]
    |   `--SEMI -> ; [2:15]
    `--RCURLY -> } [3:0]

After "INTERFACE_DEF" there are no other class/interface so no need to do separation.
Check do separation between siblings base on AST tree. There is no sibling for "INTERFACE_DEF".

Request for some functionality to enforce empty line after "{" will be addressed at #3379.

@romani romani closed this Mar 5, 2017

@romani romani removed the approved label Mar 5, 2017

romani added a commit that referenced this issue Mar 5, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 5, 2017

Member

I did minor documentation extension to make it clear in documentation.

Member

romani commented Mar 5, 2017

I did minor documentation extension to make it clear in documentation.

@robertpainsi

This comment has been minimized.

Show comment
Hide comment
@robertpainsi

robertpainsi Mar 5, 2017

Contributor

@romani, thank you very much for your answer. You perfectly summarized how the check currently works.

It seems that I failed to explain clearly how the check works and what the problems are when we force a newline after the { (see example in the description) without expanding the current check configuration. My other mistake was, that I already tried to work on the specification of the extended check.

Sorry for the circumstances. I'll take more care next time.

NoEmptyLineSeparator
please create separate issue on this. This check is simple , might be used by somebody.

Yes, I'll do that asap!

Contributor

robertpainsi commented Mar 5, 2017

@romani, thank you very much for your answer. You perfectly summarized how the check currently works.

It seems that I failed to explain clearly how the check works and what the problems are when we force a newline after the { (see example in the description) without expanding the current check configuration. My other mistake was, that I already tried to work on the specification of the extended check.

Sorry for the circumstances. I'll take more care next time.

NoEmptyLineSeparator
please create separate issue on this. This check is simple , might be used by somebody.

Yes, I'll do that asap!

@romani romani added the miscellaneous label Mar 5, 2017

@robertpainsi

This comment has been minimized.

Show comment
Hide comment
@robertpainsi

robertpainsi Mar 7, 2017

Contributor

NoEmptyLineSeparator
please create separate issue on this. This check is simple , might be used by somebody.

Ah, the NoEmptyLineSeparator as I described it could be handled sufficiently by "empty lines that wrap content of some block" with setting it to 0 lines. At least if it supports all blocks like methods/if/while/etc. https://groups.google.com/forum/#!topic/checkstyle/KZn_d8nC4c0

@embee1981, if you want to enforce an empty line after each '{' but not before '}', take a look at #3379 (comment) Just adapt the regex for '{' accordingly.
If a regex still won't do it because you need more fine tuning, please create a new issue and propose it as a new check. This issue will just address the EmptyLineSeparator check which works as specified.
I guess you may want something like 'empty line before first child'.

Contributor

robertpainsi commented Mar 7, 2017

NoEmptyLineSeparator
please create separate issue on this. This check is simple , might be used by somebody.

Ah, the NoEmptyLineSeparator as I described it could be handled sufficiently by "empty lines that wrap content of some block" with setting it to 0 lines. At least if it supports all blocks like methods/if/while/etc. https://groups.google.com/forum/#!topic/checkstyle/KZn_d8nC4c0

@embee1981, if you want to enforce an empty line after each '{' but not before '}', take a look at #3379 (comment) Just adapt the regex for '{' accordingly.
If a regex still won't do it because you need more fine tuning, please create a new issue and propose it as a new check. This issue will just address the EmptyLineSeparator check which works as specified.
I guess you may want something like 'empty line before first child'.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 10, 2017

Member

discussion is continued ... please join to https://groups.google.com/forum/#!topic/checkstyle/KZn_d8nC4c0 to share ideas on design of new Check

Member

romani commented Mar 10, 2017

discussion is continued ... please join to https://groups.google.com/forum/#!topic/checkstyle/KZn_d8nC4c0 to share ideas on design of new Check

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 18, 2017

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