Update for existing Check: WhitespaceAround

[maxvetrenko]

Need to update existing Check to allow empty classes or interfaces.

public class Foo {}

public interface Foo {}

Need to ignore missing whitespaces in annotations:

@MapFeature.Require({ALLOWS_NULL_KEYS, ALLOWS_NULL_VALUES})

Need to allow empty for cycle:

for (first = 0; first < len && matches(sequence.charAt(first)); first++) {}

[romani]

An empty block or block-like construct may be closed immediately after it is opened, with no characters or line break in between ({}), unless it is part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else or try/catch/finally).

"... may be closed immediately ...." = no rule, it is up to a user.

"unless it is part of a multi-block statement" --> this is must rule .

So you have a rule to implement - NonEmptyBlocks that will check that blocks of IF-ELSE or IF-ELSE_IF-ELSE and TRY-CATCH or TRY-CATCH-FINALLY or TRY-FINALLY could not be empty.

[maxvetrenko]

I understand you position. So, does this WhitespaceAround Check cover Google requirements? I investigated input tokens and I think that yes, this Check covers all Google requirements.

[maxvetrenko]

http://checkstyle.sourceforge.net/config_blocks.html#EmptyBlock
I found this Check and it can help to cover Google rule

[maxvetrenko]

We have to update this Check to cover Horizontal whitespace. Now this Check doesn't cover Annotations:

@SomeAnnotation({a, b}) (no space is used)
String[][] x = {{"foo"}}; (no space is required between {{, by item 8 below)

[maxvetrenko]

Implemented for empty classes, interfaces and for/while cycles. But I could not reproduce cases for annotations and arrays:

@SomeAnnotation({a, b}) (no space is used)
String[][] x = {{"foo"}}; (no space is required between {{, by item 8 below)

[rdiachenko]

/* Whether or not empty classes and interfaces are allowed/

What about enums?

private boolean mAllowEmptyCycles;

Change to mAllowEmptyLoops

return mAllowEmptyCycles

•        && (emptyBlockCheck(aAST, aParentType, TokenTypes.LITERAL_FOR)
  

•                || emptyBlockCheck(aAST,
  

•                        aParentType, TokenTypes.LITERAL_WHILE));
  

What about do-while?

Test if the given DetailAST is part of an empty block.

• \* An example empty block might look like the following
  

Change to:
Tests if a given DetailAST is a part of an empty block.
The example of an empty block might be as follows:

(emptyTypeCheck(aAST, parentType, TokenTypes.CLASS_DEF)

•                    || emptyTypeCheck(aAST,
  

•                            parentType, TokenTypes.INTERFACE_DEF))
  

This logic should be encapsulated in the emptyTypeCheck method. The definition of this method should be as follows:
private boolean emptyTypeCheck(DetailAST aAST, int aParentType)

[rdiachenko]

Also update xdocs

[maxvetrenko]

return mAllowEmptyCycles
&& (emptyBlockCheck(aAST, aParentType, TokenTypes.LITERAL_FOR)
|| emptyBlockCheck(aAST,
aParentType, TokenTypes.LITERAL_WHILE));
What about do-while?

Don't understand

[maxvetrenko]

The rest is done.

[rdiachenko]

Whether or not empty cycles are allowed

Sometimes you use "cycles", somethimes - "loops". Fix it

Check for allowed empty method, ctor or cycle blocks.

Should be "Checks if empty methods, ctors or loops are allowed."

Check for allowed empty classes and interfaces

Should be "Checks if empty classes, interfaces or enums are allowed."

return mAllowEmptyCycles
&& (emptyBlockCheck(aAST, aParentType, TokenTypes.LITERAL_FOR)
|| emptyBlockCheck(aAST,
aParentType, TokenTypes.LITERAL_WHILE));
What about do-while?

Don't understand

I meant this case: do {} while (...);

final DetailAST grandParent = aAST.getParent().getParent();

•  int match = grandParent.getType();
  

Something wrong with your logic. I'm not sure that you'll get the right parent for every input ast, moreover won't you get NPE in case you don't have a second parent?

[maxvetrenko]

I think, that such implementation will be more stable: commit

I meant this case: do {} while (...);

I understand this. I could't understand how do-while loop can use in this case. Let's have a call

The rest is done

[rdiachenko]

this check can be configured to allow empty method, types,

Change to "this check can be configured to allow empty methods, types,"

To configure the check to allow empty types blocks use

Change to "To configure the check to allow empty type blocks use"

To configure the check to allow empty loops blocks use

Change to "To configure the check to allow empty loop blocks use"

Sets whether or not empty types bodies are allowed

Change to "Sets whether or not empty type bodies are allowed"

Sets whether or not empty loops bodies are allowed.

Change to "Sets whether or not empty loop bodies are allowed."

final int type = aAST.getType();

Change to "currentType"

final DetailAST grandParent = aAST.getParent().getParent();

Change to "typeNode"

final int match = grandParent.getType();

Change to "type"

public void testEmptyTypesAndCycles() throws Exception

Each test has to be small and has to have only one responsibility. Devide this test onto: testEmptyTypeBlock and testEmptyLoopBlock

[rdiachenko]

Please rollback your changes in pom.xml. They shouldn't be present in the pull request

[romani]

methods that are return boolean have to start with "is" "are" - to be a question like in English

emptyTypeCheck ====> isEmptyType
emptyLoopCheck ====> isEmptyLoop

do not follow bad practice that already exists in this Check (emptyCtorBlockCheck).
Please rename that methods too.
emptyCtorBlockCheck ===> isEmptyCtorBlock

[romani]

isEmptyCtroBlock ??? What is Ctro ?

[maxvetrenko]

methods that are return boolean have to start with "is" "are" - to be a question like in English

Done.

[romani]

for, while, do-while loops constructor bodies.
==>
for, while, do-while loops and constructor bodies.

[romani]

the rest look good, please provide PR and make a put in PR to your "testproject" where you covered by test whole "4.6.2 Horizontal whitespace"

[maxvetrenko]

Merged