new rule: enforce empty lines that wrap content of some block

[Roman Ivanov]

continuation from https://groups.google.com/forum/#!topic/checkstyle/rzNJDM_uvyg

class MyClass {

 

   public void method1() {}

   public void method2() {}

   public void method3() {}

}

users want to enforce empty line separator after "{" and before "}" of class.

But looks like there is demand for wrapping policy in all curly braces, .... to enforce empty files or to enforce no empty lines.

from Rveach:

Like issue I pointed to stated, eventually people wanted to remove blank lines after/before all braces. I included method/if/white/etc, because they can have braces and I agree that I don't want to see blank lines after them. Like demonstrated in issue, I use regular expressions to control it right now.
Creator of this post wanted just to control classes, while I am looking to control all braces. With customizable tokens, it would be easy to make these other ones optional and let user configure how he wanted them without much programming overhead. Also, it doesn't deviate from the purpose of the check proposed thus far, as it just extends which type of braces it applies to.

That is correct. Piotr requested blank lines. I am requesting no blank lines.
That's why I like the idea of a new check to set an option, new line or no new line. Similar to other block checks like Left/Right curly. Allows everyone to customize every detail of how they want it.

-----------------------

from Rveach:

after method brace: https://github.com/checkstyle/checkstyle/blob/d4bec82ed1be6191cea18cc7effeef201d6ab907/src/main/java/com/puppycrawl/tools/checkstyle/api/AutomaticBean.java#L184 (2 others in this class)
after if brace: https://github.com/checkstyle/checkstyle/blob/d4bec82ed1be6191cea18cc7effeef201d6ab907/src/main/java/com/puppycrawl/tools/checkstyle/api/DetailAST.java#L341
after if brace: https://github.com/checkstyle/checkstyle/blob/d4bec82ed1be6191cea18cc7effeef201d6ab907/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L409
after while brace: https://github.com/checkstyle/checkstyle/blob/d4bec82ed1be6191cea18cc7effeef201d6ab907/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/LeftCurlyCheck.java#L290 (2 others for if in this file)
after if brace: https://github.com/checkstyle/checkstyle/blob/d4bec82ed1be6191cea18cc7effeef201d6ab907/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/AbstractIllegalMethodCheck.java#L71
after switch brace: https://github.com/checkstyle/checkstyle/blob/d4bec82ed1be6191cea18cc7effeef201d6ab907/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheck.java#L328
after try brace: https://github.com/checkstyle/checkstyle/blob/d4bec82ed1be6191cea18cc7effeef201d6ab907/src/test/java/com/puppycrawl/tools/checkstyle/BaseCheckTestSupport.java#L254

The list goes on and on. You can see all matches with the regular expression `{\r\n\s*\r\n`. I see almost 500 matches. Since there are so many, I can't see how this isn't their style and it shows just how inconsistent checkstyle's code is in regards to blank lines after a brace. I prefer uniformity. We should set one way of doing things and everyone follows. I feel this check will help us in this area.

[Roman Ivanov]

similar issue on github - https://github.com/checkstyle/checkstyle/issues/3923 that describe request the same as from Piotr (only for classes)

[Roman Ivanov]

> You can see all matches with the regular expression `{\r\n\s*\r\n`. I see almost 500 matches

here is out put for Checkstyle sources https://gist.github.com/romani/19318e2dfc2b74b64df6d4388a166d05

[Roman Ivanov]

Emptyline wrapping for classes is very easy case.

My concern was with wrapping all other blocks, as there are multiple nuances to make code pretty and ease to read.

Some my notes from past .... incomplete ideas .... :

Place a empty line after multy line method declaration ?

    public static String buildSetColumns(Collection<Column> columns, final String leftAlias,

            final String rightAlias) {

        return Joiner.on(", ").join(transform(columns, new Function<Column, String>() {

            @Override

            public String apply(Column input) {

                return input.quoted(leftAlias) + " = " + input.quoted(rightAlias);

            }

        }));

    }

    public static String buildColumnsEquals(Collection<Column> columns, final String leftAlias,

            final String rightAlias, boolean buildNotEquals) {

        Collection<ImmutableList<Column>> columnPairs = zipCollections(columns, columns);

        return buildColumnPairsEquals(columnPairs, leftAlias, rightAlias, buildNotEquals);

    }

    public static String buildColumnPairsEquals(Collection<ImmutableList<Column>> columnPairs,

            final String leftAlias, final String rightAlias, final boolean buildNotEquals) {

        return Joiner.on(buildNotEquals ? " or " : " and ").join(

                transform(columnPairs, new Function<ImmutableList<Column>, String>() {

                    @Override

                    public String apply(ImmutableList<Column> input) {

                        return buildColumnsEquals(input.get(0), input.get(1), leftAlias, rightAlias, buildNotEquals);

                    }

                }));

    }

Or it is question of method declaration indentation (the same code below):

    public static String buildColumnsEquals(Collection<Column> columns, final String leftAlias,

                                            final String rightAlias, boolean buildNotEquals) {

        Collection<ImmutableList<Column>> columnPairs = zipCollections(columns, columns);

        return buildColumnPairsEquals(columnPairs, leftAlias, rightAlias, buildNotEquals);

    }

    public static String buildColumnPairsEquals(Collection<ImmutableList<Column>> columnPairs,

                            final String leftAlias, final String rightAlias, final boolean buildNotEquals) {

        return Joiner.on(buildNotEquals ? " or " : " and ").join(

                transform(columnPairs, new Function<ImmutableList<Column>, String>() {

                    @Override

                    public String apply(ImmutableList<Column> input) {

                        return buildColumnsEquals(input.get(0), input.get(1), leftAlias, rightAlias, buildNotEquals);

                    }

                }));

    }

    public static String buildColumnsEquals(Column leftColumn, Column rightColumn, String leftAlias, String rightAlias,

                                            boolean buildNotEquals) {

        if (leftColumn == null || rightColumn == null) {

            throw new IllegalArgumentException("leftColumn and rightColumn should not be null");

        }

-----------------------

    private void applyCumulativeUpdate(ImmutableSortedSet<StateEntry> allStatesToApply, WorkMode workMode)

            throws SQLException {

        boolean fullReload = workMode == WorkMode.RELOAD_FROM_SCRATCH;

        final Connection connection = dataSource.getConnection();

----------------------------

    private static void countRecordsInCumulativeTables(Connection connection, TableChangeChecker checker,

            MultiFeedDefinition multiFeedDefinition) throws SQLException {

        LOG.info("Calculating number of insert/update/delete records in cumulative update");

        for (TableName tableName : multiFeedDefinition.getTableNamesWithSchema()) {

            TableName cumulativeTable = multiFeedDefinition.getCumulativeTable(tableName).get();

            int inserts = countRecords(connection, cumulativeTable, Action.INSERT.getValue(), INDEX_CUMULATIVE);

            int updates = countRecords(connection, cumulativeTable, Action.UPDATE.getValue(), INDEX_CUMULATIVE);

            int deletes = countRecords(connection, cumulativeTable, Action.DELETE.getValue(), INDEX_CUMULATIVE);

            checker.addExpectedChange(CUMULATIVE_STATE, tableName, inserts, updates, deletes);

        }

even from code of checkstyle that Richard point to , you can see that empty lines at the top of block is used to clearly separate header(for/while/if/method/...) of block with following code. Empty lines are good when method signatures are long and wrapping rules are a matching the code in block. But this is very subjective view on a code. I do not like messy code.

[Roman Ivanov]

As community raising issue again and again ..... We should provide some basic Check first and when think (if smb except of me care) how to enforce new line at the top of block in smart way.

So for now let do simple Check that demand or forbid new line. 

Please share your ideas on design/names.

My first proposal:

BlockEmptyLineWrappingCheck

tokens= [CLASS, METHOD, IF, ...........] (by default only class)

topSeparator = [empty-line | empty-line-allowed | no-empty-line]

bottomSeparator = [empty-line | empty-line-allowed | no-empty-line]

empty-line-allowed == mean no validation.

If you consider all my examples above, we could make config for methods to allow user's empty line at the top and completely forbid it at the bottom.

BlockEmptyLineWrappingCheck

tokens= "METHOD_DEF"

topSeparator = "empty-line-allowed"

bottomSeparator = "no-empty-line"

please share your ideas, I do not very like name I suggested.

[Piotr Listkiewicz]

Ok, i didnt notice recipient in email

2017-03-10 7:39 GMT+01:00 Roman Ivanov <romani...@gmail.com>:

Please reply to mail list, sorry for mess in email.

On Mar 9, 2017 10:16 PM, "Piotr Listkiewicz" <piotr.li...@gmail.com> wrote:

My first proposal:
BlockEmptyLineWrappingCheck
tokens= [CLASS, METHOD, IF, ...........] (by default only class)
topSeparator = [empty-line | empty-line-allowed | no-empty-line]
bottomSeparator = [empty-line | empty-line-allowed | no-empty-line]
empty-line-allowed == mean no validation.
If you consider all my examples above, we could make config for methods to allow user's empty line at the top and completely forbid it at the bottom.
BlockEmptyLineWrappingCheck
tokens= "METHOD_DEF"
topSeparator = "empty-line-allowed"
bottomSeparator = "no-empty-line"

I find this proposal reasonable, maybe name of this check could be better but i think thats one of the things that could be discussed in code review. Roman let me know when idea of this check is discussed enough so I can send PR(i am interested in having this check so i would like to work on it)

[R Veach]

I am fine with your proposal too.