Consider enum trailing commas in SeparatorWrap semicolons

[cypai]

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

~/work/test $ javac Test.java
~/work/test $ cat Test.java
public enum Test {
    A,
    B,
    ; // Would prefer no violation here

    public void foo() {
        bar()
        ; // Violation here is fine
    }

    public void bar() {}
}
~/work/test $ cat config.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!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="SeparatorWrap">
            <property name="tokens" value="SEMI"/>
        </module>
	</module>
</module>
~/work/test $ java -jar checkstyle-7.4-all.jar -c config.xml Test.java
Starting audit...
[WARN] /home/cpai/work/test/Test.java:4:5: ';' should be on the previous line. [SeparatorWrap]
[WARN] /home/cpai/work/test/Test.java:8:9: ';' should be on the previous line. [SeparatorWrap]
Audit done.

It would be nice if SeparatorWrap would ignore the semicolon if it is on the line after a trailing comma in enums.

[romani]

problem is part of misconception that Separator related Check use operator wrapping model - http://checkstyle.sourceforge.net/property_types.html#wrapOp

It is be good to have the same as in http://checkstyle.sourceforge.net/config_whitespace.html#NoWhitespaceBefore allowLineBreaks property.

@rnveach , please share your ideas on this issue.

[rnveach]

@romani

It is be good to have allowLineBreaks property.

Yes, this property would not print violations for both cases. I am fine with doing that, but it seems the user might prefer a violation in one of these cases.

B,
; // Would prefer no violation here
bar()
; // Violation here is fine

We can't meet this expectation with this check. The problem with these type of checks is they do a blind token match. Both areas are semi-colons and AST tree only knows them as a semi-colon. User wants that token but in a specific context - allow for enum definitions but disallow in field/method. If we did make some type of override, we would need to expand it to other areas like for loops and such.
To get specifically what the user wants, it seems to me we would need to create a new check where user has greater control over the token's context instead of a blind match.

[romani]

Issue is related to #3766 by used code.

@cypai ,
do you expect violation on code like below? (the same formatting is used to minimize changes):

builder.setA()
       .setB()
       .build()
       ;

This SeparatorWrap is general simple Check, we can not introduce in it logic for special ";" processing in different contexts, new Check should be created.

[cypai]

@romani
A violation there is fine.

For builders, having the semicolon at .build(); is fine since the git diff would be at .setB().

[romani]

@cypai , consider any other chain calls

collection.filer()
   .map()
   .filter()
   ;

keep in mind that you might have the same reason for such formatting as you you has in enum.
If I miss smth please share with me reason of formatting in Enum that you provided.

[romani]

@cypai , ping !

[cypai]

The main reason I had for this is similar to trailing commas in Arrays. It's easy to add/delete/sort elements by line, and if I do so at the end of the declaration, I won't need to go back to delete the semicolon, etc.

public enum Test {
    A,
    B,
    ;    // If I want to add a new element C, I can simply add the line before this
}

vs

public enum Test {
    A,
    B;    // If I want to add a new element C, I need to delete the semicolon,
          // then add C and a semicolon to the next line
}

So it's pretty much just a convenience issue.

For chain calls, I would prefer the semicolon on the same line. The last line in chain calls is typically a terminal operation (like in builders or Java 8 streams), and therefore wouldn't have the same convenience issue when the semicolon is on the same line. If it is not a chain call like a builder or stream, it's probably a more complicated change than simply moving a semicolon around.

However, I do agree that this extra logic probably should not be part of this simple check. It might be better to make it an experimental check.

Most likely I will just keep using the current check and leave trailing commas in enums as a violation.

[romani]

There are such idea already sevntu-checkstyle/sevntu.checkstyle#464
It is even almost implemented sevntu-checkstyle/sevntu.checkstyle#474 but looks like abandoned on the last step.

If smb have time to finish , please feel free.