IndentationCheck_issue766

[Alexey Zuy]

Check: link

Reports generated for guava project with only IndentationCheck enabled and configured the same way it configured in google_checks.xml:

report for old check implementation: link
report for new check implementation: link

please review.

[Roman Ivanov]

Hi Alexey,

Please share link to commit, i need to see diff (changes) from cell phone.

Please look at http://checkstyle.sourceforge.net/reports/google-style/guava/ there we have only 304 warnings, please investigate why we so different.

Thanks,
Roman Ivanov

[Alexey Zuy]

Hi Roman,

squashed all commits into one: link

Please look at http://checkstyle.sourceforge.net/reports/google-style/guava/ there we have only 304 warnings, please investigate why we so different.

It looks like this report was generated against guava implementation sources(without UT sources).
I removed following folders and generated report. Amount of violations from IndentationCheck was close to 304(as it is in report at checkstyle.sourceforge.net):
guava-tests
guava-gwt/test
guava-gwt/test-super
guava-testlib/test


thanks,
Zuy Alexey

[Roman Ivanov]

Hi Alexey,

> Amount of violations from IndentationCheck was close to 304

I do not like that, any difference could mean regression (by fixing one problem, you cause another problem)

Please checkout guava code to the date 2015-02-28 as it is mentioned in our repo.

Any (even one) difference in violations should be explained, please share HTML report.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Andrew,

1) Please separate fixes that you did for Check and for code of checkstyle to pass "verify" stage.

Verification of such huge update is impossible.

2) I am worry about changes at VisibilityModifierCheck.java file for "private static final List<String> DEFAULT_IMMUTABLE_TYPES = ImmutableList.of("

and other changes for  indention of close ")". We need to sure that such a formatting does not brake major IDE settings for indentation. I see that Eclipse is supporting that, but what about Idea and NetBeans ?

thanks,

Roman Ivanov

​

[Alexey Zuy]

Hi Roman,

Separeted IndentationCheck fixes from code of checkstyle fixes.
link to Check changes

thanks,
Zuy Alexey

[Roman Ivanov]

Hi Alexey,

While I am reviewing a code please update code coverage (most likely inputs need to be extended).

As we recently agreed, we do not reduce code coverage after each update, it could stay the same or grow to 100.

thansk,

Roman Ivanov

​

[Alexey Zuy]

Hi Roman,

1) Investigation of formatting preferences in common IDEs:

intelliJidea:
Closing parenthesis (")") has the same indentation as opening one. There is no way to change this behavior.

int i  = Integer.valueOf( //indent:0
    "13" //indent:8
);  //indent:0

netbeans:
Closing parenthesis has the same indentation as opening one. There is no way to change this behavior.

int i = Integer.valueOf( //indent:0
    "13" //indent:4
);  //indent:0

2) Reports generated against guava implementation sources(without UT sources) for guava state at 28.02.2015(as in report at sourceforge).
before

after

thanks,
Zuy Alexey

[Roman Ivanov]

Hi Alexey,

I see that in  Intellijidea location of ")" is partly configurable. See picture below. It either on new line with indentation of line with "(", or it is on same line as last argument:

​

I cannot find how to configure that in Eclipse. Do you know where is it ?

I see that Eclispe only allow brace to be on the same line as last parameter:

​

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Alexey, 

review of "After" report ....

1) 

guava/guava-gwt/src-super/com/google/common/collect/super/com/google/common/collect/Lists.java

Severity Category Rule Message Line

 Warning indentation Indentation 'cartesianProduct' have incorrect indentation level 6, expected level should be 7. 411

 Warning indentation Indentation 'cartesianProduct' have incorrect indentation level 6, expected level should be 7. 471

Can you explain why level should be 7 ? There 3 cases like that in report.

2) 

> AbstractCollectionTestSuiteBuilder.java

MapTestSuiteBuilder.java

That is sign that we can not use such indentaion rule for ")", whole world does not do that :(.

3)

all other differences looks good, they are valid violations.

thanks,

Roman Ivanov

​

[Alexey Zuy]

Hi Roman,

Location of ")" parenthesis:
NetBeans, as well as Eclipse, has no configuration options to place ")" on new line:

So, yes, closing paren must have the same indentation level as opening paren. This policy will not conflict with common IDEs.

1) This is specifics of Check implementation. Method definition starts at line 410:

   */ static <B> List<List<B>>
      cartesianProduct(List<? extends List<? extends B>> lists) {
    return CartesianList.create(lists);

indentation of line 410 is indentation of leading asterix, which is 3. So line wrap indent is calculated as 3 + 4 = 7.

thanks,

Zuy Alexey

[Roman Ivanov]

Hi Alexey,

> indentation of line 410 is indentation of leading asterix, which is 3. So line wrap indent is calculated as 3 + 4 = 7.

from Google style:

> 4.8.4.1 Indentation 

As with any other block, the contents of a switch block are indented +2.

After a switch label, a newline appears, and the indentation level is increased +2, exactly as if a block were being opened. The following switch label returns to the previous indentation level, as if a block had been closed.

base indentation as 3 is not expected, it should be 2 or 4 or .... . Please recheck why we do not put a violation on method declaration.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Alexey,

https://github.com/pirat9600q/checkstyle/blob/c6679ea868845ab6d3e94da4fe90b02d2ca29190/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/MethodCallLineWrappingHandler.java

1) 

> private void checkMethodCallTargetChainWrapping()

private static List<DetailAST> getMethodCallTargetChain(DetailAST methCall)

ONE RETURN point from all methods !!

============

https://github.com/pirat9600q/checkstyle/blob/c6679ea868845ab6d3e94da4fe90b02d2ca29190/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/LineWrappingHandler.java

2) 

> * @author maxvetrenko

as you did significant changes, please add yourself to authors

3)

> protected final .........

What is a reason of make it protected (intention to use in child classes) and FINAL (forbidding to override) ?

4) 

Please make a order in Class: fields, abstract methods, C-tors, setters, ..... static private methods. 

5) 

> final char c = line.charAt(start);

never use one-char names for variables, user "chr" at least.

6) 

> protected final int getLineStart(String line)

> protected static List<DetailAST> getNodeChildren(DetailAST node)

ONE RETURN !

7)

> protected static

What a reason ? please read http://stackoverflow.com/questions/16617408/overriding-static-methods-in-java

8) 

> private static boolean isExceptionalTokenType(DetailAST node)

make it one line.

thanks,

Roman Ivanov

[Alexey Zuy]

Hi Roman,

last commit:
commit

thanks,
Zuy Alexey