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: incorrect expectation for wrapped arguments of chained calls #3208

Closed
eric-milles opened this Issue May 22, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@eric-milles

eric-milles commented May 22, 2016

We are seeing warnings for wrapped lines of our builder calls. For eaxmple:

    public static final ResultType WHATEVER = ResultType.builder("WHATEVER") //$NON-NLS-1$
        .passThrough(true)
        .contentType("WHATEVER") //$NON-NLS-1$
        .supportedMetadataTasks(
            TaskOne.RESULT,
            TaskTwo.RESULT,
            TaskThree.RESULT)
        .build();

This results in warnings for each line of the varargs method call. The warning is seeking the lines to be indented at the same level as the methods (one less tab each).

We use the indentation check in this way:

        <module name="Indentation">
            <message key="indentation.error" value="Line should be indented by {2} spaces" />
            <message key="indentation.child.error" value="Line should be indented by {2} spaces" />
            <message key="indentation.error.multi" value="Line should be indented by one of: {2}" />
            <property name="forceStrictCondition" value="true" />
            <property name="basicOffset" value="4" />
            <property name="caseIndent" value="0" />
        </module>
@kevinconaway

This comment has been minimized.

Show comment
Hide comment
@kevinconaway

kevinconaway Jun 17, 2016

Contributor

@rnveach I'm seeing something similar as well when working with Apache Spark. I've created a small test class that exposes this issue:

public class Test {

    public static void main(String [] args) {
        MockRDD mockRDD = new MockRDD();
        mockRDD.mapToPair(
            new ToSequenceFilePair()
        ).partitionBy(
            new TimeBasedPartitioner(partitions)
        ).saveAsHadoopFile(
            outputPath.toString(),
            LongWritable.class,
            BytesWritable.class,
            PartitionedOutputFormat.class,
            Lz4Codec.class
        );
    }

    private static class MockRDD {

        public MockRDD mapToPair(Object arg) {
            return this;
        }

        public MockRDD partitionBy(Object arg) {
            return this;
        }

        public MockRDD saveAsHadoopFile(Object... args) {
            return this;
        }

    }

}

The checkstyle rule config:

<module name = "Checker">

    <module name="TreeWalker">
        <module name="Indentation"/>
    </module>

</module>

Running this on the CLI gives me:

java -jar checkstyle-6.19-all.jar -c checkstyle.xml Test.java
Starting audit...
[ERROR] /Users/kconawa/Downloads/Test.java:12: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:13: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:14: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:15: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:16: 'method call rparen' have incorrect indentation level 8, expected level should be 12. [Indentation]
Audit done.
Checkstyle ends with 5 errors.

If I change the code to the indentation below it passes but it is clearly incorrect:

        mockRDD.mapToPair(
            new ToSequenceFilePair()
        ).partitionBy(
            new TimeBasedPartitioner(partitions)
        ).saveAsHadoopFile(
            outputPath.toString(),
                LongWritable.class,
                BytesWritable.class,
                PartitionedOutputFormat.class,
                Lz4Codec.class
            );
Contributor

kevinconaway commented Jun 17, 2016

@rnveach I'm seeing something similar as well when working with Apache Spark. I've created a small test class that exposes this issue:

public class Test {

    public static void main(String [] args) {
        MockRDD mockRDD = new MockRDD();
        mockRDD.mapToPair(
            new ToSequenceFilePair()
        ).partitionBy(
            new TimeBasedPartitioner(partitions)
        ).saveAsHadoopFile(
            outputPath.toString(),
            LongWritable.class,
            BytesWritable.class,
            PartitionedOutputFormat.class,
            Lz4Codec.class
        );
    }

    private static class MockRDD {

        public MockRDD mapToPair(Object arg) {
            return this;
        }

        public MockRDD partitionBy(Object arg) {
            return this;
        }

        public MockRDD saveAsHadoopFile(Object... args) {
            return this;
        }

    }

}

The checkstyle rule config:

<module name = "Checker">

    <module name="TreeWalker">
        <module name="Indentation"/>
    </module>

</module>

Running this on the CLI gives me:

java -jar checkstyle-6.19-all.jar -c checkstyle.xml Test.java
Starting audit...
[ERROR] /Users/kconawa/Downloads/Test.java:12: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:13: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:14: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:15: 'method call' child have incorrect indentation level 12, expected level should be 16. [Indentation]
[ERROR] /Users/kconawa/Downloads/Test.java:16: 'method call rparen' have incorrect indentation level 8, expected level should be 12. [Indentation]
Audit done.
Checkstyle ends with 5 errors.

If I change the code to the indentation below it passes but it is clearly incorrect:

        mockRDD.mapToPair(
            new ToSequenceFilePair()
        ).partitionBy(
            new TimeBasedPartitioner(partitions)
        ).saveAsHadoopFile(
            outputPath.toString(),
                LongWritable.class,
                BytesWritable.class,
                PartitionedOutputFormat.class,
                Lz4Codec.class
            );
@kevinconaway

This comment has been minimized.

Show comment
Hide comment
@kevinconaway

kevinconaway Jun 24, 2016

Contributor

@Vladlis is this something that you would be able to take a look at?

Contributor

kevinconaway commented Jun 24, 2016

@Vladlis is this something that you would be able to take a look at?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 27, 2016

Member

@rnveach , really weird behaviour now, definitely a bug.

Member

romani commented Jun 27, 2016

@rnveach , really weird behaviour now, definitely a bug.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 27, 2016

Member

Yes. Output has not changed since before 5.9, so this has been around for a while.

Issue is most likely because we call the LineWrapping on the entire method call: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/MethodCallHandler.java#L166
I will have to see how each IDE handles it by default.

Member

rnveach commented Jun 27, 2016

Yes. Output has not changed since before 5.9, so this has been around for a while.

Issue is most likely because we call the LineWrapping on the entire method call: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/MethodCallHandler.java#L166
I will have to see how each IDE handles it by default.

kevinconaway added a commit to kevinconaway/checkstyle that referenced this issue Jul 6, 2016

kevinconaway added a commit to kevinconaway/checkstyle that referenced this issue Jul 6, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 9, 2016

Member

Code from PR.

        MockRDD mockRDD = new MockRDD(); //indent:8 exp:8
        mockRDD.mapToPair( //indent:8 exp:8
            null //indent:12 exp:12
        ).saveAsHadoopFile(  //indent:8 exp:8
            null //indent:12 exp:12
        ); //indent:8 exp:8

All IDEs indent the given code the same as commented by default.

When the ( is line separated from saveAsHadoopFile, Eclipse keeps the ( at indent 4, while the other IDEs indent it to 8. Children are indented relative to parent.
Eclipse view:

        ).saveAsHadoopFile//
        ( // indent:8 exp:8
                null, // indent:12 exp:12
        ); // indent:8 exp:8

Other IDEs view:

        ).saveAsHadoopFile//
                ( //indent:8 exp:8
                        null //indent:12 exp:12
                );  //indent:8 exp:8

When the ) is line separated from .saveAsHadoopFile, all IDEs indent .saveAsHadoopFile to 8 and keep children relative to parent. Eclipse seems to be the only one that keeps the final ); at indent 4 no matter what, even when previous lines are increased in indent.
Eclipse view:

        )//
                .saveAsHadoopFile( // indent:8 exp:8
                        null, // indent:12 exp:12
        ); // indent:8 exp:8

Other IDEs view:

        )//
                .saveAsHadoopFile( //indent:8 exp:8
                        null //indent:12 exp:12
                );  //indent:8 exp:8
Member

rnveach commented Jul 9, 2016

Code from PR.

        MockRDD mockRDD = new MockRDD(); //indent:8 exp:8
        mockRDD.mapToPair( //indent:8 exp:8
            null //indent:12 exp:12
        ).saveAsHadoopFile(  //indent:8 exp:8
            null //indent:12 exp:12
        ); //indent:8 exp:8

All IDEs indent the given code the same as commented by default.

When the ( is line separated from saveAsHadoopFile, Eclipse keeps the ( at indent 4, while the other IDEs indent it to 8. Children are indented relative to parent.
Eclipse view:

        ).saveAsHadoopFile//
        ( // indent:8 exp:8
                null, // indent:12 exp:12
        ); // indent:8 exp:8

Other IDEs view:

        ).saveAsHadoopFile//
                ( //indent:8 exp:8
                        null //indent:12 exp:12
                );  //indent:8 exp:8

When the ) is line separated from .saveAsHadoopFile, all IDEs indent .saveAsHadoopFile to 8 and keep children relative to parent. Eclipse seems to be the only one that keeps the final ); at indent 4 no matter what, even when previous lines are increased in indent.
Eclipse view:

        )//
                .saveAsHadoopFile( // indent:8 exp:8
                        null, // indent:12 exp:12
        ); // indent:8 exp:8

Other IDEs view:

        )//
                .saveAsHadoopFile( //indent:8 exp:8
                        null //indent:12 exp:12
                );  //indent:8 exp:8

romani added a commit that referenced this issue Jul 29, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 30, 2016

Member

Fix is merged

Member

romani commented Jul 30, 2016

Fix is merged

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