Skip to content
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

Indentation: fix line wrap hanlding #2950

Closed
rnveach opened this issue Feb 20, 2016 · 8 comments
Closed

Indentation: fix line wrap hanlding #2950

rnveach opened this issue Feb 20, 2016 · 8 comments
Assignees
Milestone

Comments

@rnveach
Copy link
Member

rnveach commented Feb 20, 2016

$ cat TestClass.java
public class Test { // indent 0
    int var6 = 5; int var7 = 6, // indent 4 -- line #2
        var8 = 5; // indent 8 -- line #3

    public void method() { // indent 4
        long_lined_label: if (true // indent 8 -- line #6
            && true) {} // indent 12 -- line #7
    } // indent 4
    /* package-private */ static final void // indent 4 -- line #9
        method2() {} // indent 8 -- line #10
}

$ 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">
         <property name="tabWidth" value="4"/>

         <module name="Indentation">
           <property name="basicOffset" value="4"/>
         </module>
    </module>
</module>

$ java -jar checkstyle-6.15-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3: 'var8' have incorrect indentation level 8, expected level should be 22. [Indentation]
[ERROR] TestClass.java:7: '&&' have incorrect indentation level 12, expected level should be 30. [Indentation]
[ERROR] TestClass.java:10: 'method2' have incorrect indentation level 8, expected level should be 30. [Indentation]
Audit done.
Checkstyle ends with 3 errors.

Line wrapping currently handles instances wrong when it's starting node isn't the first on the line. I mentioned this in another issue where I wanted to use line wrapping, but had to make weird adjustments because I couldn't guarantee the node was the first on the line.
If the node isn't the first on the line, line wrapping thinks it is and therefore uses that indentation as the base for the next wrapped lines. It should instead use either the current line's indentation or take as input the expected indentation from previous checks.
All the cases above are wrapped correctly with 4 spaces, even though 2 (line 7 , line 10) don't follow the one statement per line rule.

Default Auto Formatters:
I used the same code listed above, with comments and all, with the IDE's default auto format options.
IntelliJ: splits lines 2, 6, 9 into separate lines (think of one statement per line rule). Line 3 and 7 keep their +4 indents after the split, line 10 is indented the same as line 9.
Eclipse: only line 2 is split into separate lines, the rest stay the same. Line 3 keeps its +4 indent after the split, lines 7 and 10 are indented to the same position as their previous line.
NetBeans: splits lines 2 and 6 into separate lines, the rest stay the same. Lines 3, 7, and 10 keep their +4 indent.

@romani
Copy link
Member

romani commented Feb 23, 2016

@rnveach ,

please append whole-code after auto-formatting from IDEs to be 100% exact what is going on. References to lines numbers helps a bit but still obscure.

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 24, 2016
@rnveach
Copy link
Member Author

rnveach commented Feb 24, 2016

NetBeans

public class Test { // indent 0

    int var6 = 5;
    int var7 = 6, // indent 4 -- line #2
            var8 = 5; // indent 8 -- line #3

    public void method() { // indent 4
        long_lined_label:
        if (true // indent 8 -- line #6
                && true) {
        } // indent 12 -- line #7
    } // indent 4

    /* package-private */ static final void // indent 4 -- line #9
            method2() {
    } // indent 8 -- line #10
}

IntelliJ:

public class Test { // indent 0
    int var6 = 5;
    int var7 = 6, // indent 4 -- line #2
            var8 = 5; // indent 8 -- line #3

    public void method() { // indent 4
        long_lined_label:
        if (true // indent 8 -- line #6
                && true) {
        } // indent 12 -- line #7
    } // indent 4

    /* package-private */
    static final void // indent 4 -- line #9
    method2() {
    } // indent 8 -- line #10
}

Eclipse:

public class Test { // indent 0
    int var6 = 5;
    int var7 = 6, // indent 4 -- line #2
            var8 = 5; // indent 8 -- line #3

    public void method() { // indent 4
        long_lined_label: if (true // indent 8 -- line #6
        && true) {
        } // indent 12 -- line #7
    } // indent 4

    /* package-private */static final void // indent 4 -- line #9
    method2() {
    } // indent 8 -- line #10
}

@romani
Copy link
Member

romani commented Feb 25, 2016

after removing non-relevant formatting:
screenshot-i2950-eclipse
screenshot-i2950-intelij
screenshot-i2950-netbeans

@romani
Copy link
Member

romani commented Feb 25, 2016

[ERROR] TestClass.java:3: 'var8' have incorrect indentation level 8, expected level should be 22. [Indentation]

We do not force user linewrap in certain point or single statement per line!!
22 is correct expected indentation. "int var7" indentation is 18.

[ERROR] TestClass.java:7: '&&' have incorrect indentation level 12, expected level should be 30. [Indentation]

We do not force user linewrap in certain point or single statement per line.
Different IDEs treat them differently - as part of one statement (Eclipse) and two statements (NETBeans, Intelij).
We need to deactivate line wrapping in IDEs and redo formatting again for all 3 IDEs. We need to have in formatting options only indentation options, nothing else.
For now indentation "30" is questionable ....

[ERROR] TestClass.java:10: 'method2' have incorrect indentation level 8, expected level should be 30. [Indentation]

Indentation 30 is incorrect for sure. Netbeans want to indent wrapped lines. Eclipse and Intelij are not indenting wrapped method signature.
I prefer Netbeans behaviour, but we need to investigate why Eclipse and Intelij do this. We could file bug on them or ask at stackoverflow.

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 27, 2016
@rnveach
Copy link
Member Author

rnveach commented Mar 21, 2016

TestClass.java:3

22 is correct expected indentation

I disagree.
My main reason is because I think forcing code to the right more defeats the main purpose of the formatter, which is to make the code look nice and easily readable. Imagine if the violation wanted column 70, you will now have some code on column 8 and some on 70, which I think doesn't read well.
Also, the behavior of most of our indentation check, as well as custom settings in external formatters, is that we increase/decrease future indentations based on where the current line starts, which it seems to more naturally say that it is the first non-whitespace character.
That is why I feel all the formattings in the first post are correct.

We need to deactivate line wrapping in IDEs

I will work on this.

TestClass.java:10

we need to investigate why Eclipse and Intelij do this

Eclipse can be customized to duplicate this if you play around with its customizations.

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 24, 2016
@rnveach
Copy link
Member Author

rnveach commented Apr 16, 2016

@romani

deactivate line wrapping in IDEs

As an example, are you expecting var6 and the next statement (var7) to not be separated as all the IDEs are breaking them apart?

In NetBeans, I turned all wrapping to Never and the formatted code is still the same as my original post. I am seeing the same results in Eclipse when I set everything to Do not wrap.

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 23, 2016
@romani romani added the bug label Apr 26, 2016
@romani romani added this to the 6.18 milestone Apr 26, 2016
@rnveach
Copy link
Member Author

rnveach commented May 27, 2016

I am closing this since the commit was merged.

@rnveach rnveach closed this as completed May 27, 2016
@romani
Copy link
Member

romani commented Jun 3, 2016

yes, commit is merged.

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

No branches or pull requests

2 participants