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

IndentationCheck: catch child indentation not checked #3803

Closed
pbludov opened this Issue Feb 7, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@pbludov
Collaborator

pbludov commented Feb 7, 2017

$cat TestClass.java

public class TestClass {
    void method() {
        try {
            if (SomeVariableWithVeryLongName
            | OtherVariableWithVeryLongName) {
            }
        }
        catch (SomeExceptionWithVeryLongName
        | OtherExceptionWithVeryLongName e) {
        }
    }
}

$ cat TestConfig.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">
  <property name="charset" value="UTF-8"/>
  <module name="TreeWalker">
    <module name="Indentation"/>
  </module>
</module>
$ java -jar checkstyle-7.5.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:5: 'if' child have incorrect indentation level 12, expected level should be 16. [Indentation]
Audit done.
Checkstyle ends with 1 errors.

There must be two indentation errors: one for 'if', another for 'catch'.
It seems like the 'catch' handler lacks some code.


@rnveach rnveach added the indentaion label Feb 7, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 7, 2017

Member

The catch is being checked, but its expected indentation is 8 instead of probably what user expects to be 12.
[ERROR] TestClass.java:9: 'catch' child have incorrect indentation level 0, expected level should be 8. [Indentation]

Member

rnveach commented Feb 7, 2017

The catch is being checked, but its expected indentation is 8 instead of probably what user expects to be 12.
[ERROR] TestClass.java:9: 'catch' child have incorrect indentation level 0, expected level should be 8. [Indentation]

@romani romani added the approved label Feb 7, 2017

@rnveach rnveach self-assigned this Feb 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 8, 2017

romani added a commit that referenced this issue Feb 8, 2017

@romani romani added the bug label Feb 8, 2017

@romani romani added this to the 7.6 milestone Feb 8, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 8, 2017

Member

fix is merged

Member

romani commented Feb 8, 2017

fix is merged

@romani romani closed this Feb 8, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 8, 2017

Member

@rnveach , please add UTs for IF expression indentation as reported in issue.
We should collect in our UTs all user reported code.

Member

romani commented Feb 8, 2017

@rnveach , please add UTs for IF expression indentation as reported in issue.
We should collect in our UTs all user reported code.

@romani romani reopened this Feb 8, 2017

bamapookie added a commit to bamapookie/checkstyle that referenced this issue Feb 10, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

@romani We have examples similar to this in valid cases, no invalid ones that I could find. You still want me to add it?

if ((j == 2 && k == 3) //indent:8 exp:8
|| test) { //indent:14 exp:>=12

Member

rnveach commented Feb 13, 2017

@romani We have examples similar to this in valid cases, no invalid ones that I could find. You still want me to add it?

if ((j == 2 && k == 3) //indent:8 exp:8
|| test) { //indent:14 exp:>=12

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 13, 2017

Member

Due to complexity of this check, it is better to have both valid and invalid cases. This will help us to catch regression on UT level.

Member

romani commented Feb 13, 2017

Due to complexity of this check, it is better to have both valid and invalid cases. This will help us to catch regression on UT level.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 16, 2017

Member

final fix is merged.

Member

romani commented Feb 16, 2017

final fix is merged.

@romani romani closed this Feb 16, 2017

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