IndentationCheck: incorrect validation for imports and package #2618

Closed
rnveach opened this Issue Nov 18, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Nov 18, 2015

D:> type config.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="ASCII" />
    <property name="severity" value="error" />
    <property name="fileExtensions" value="java" />

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

D:> type test.java

package com.puppycrawl.tools.checkstyle.checks.indentation; //indent:0 exp:0

 import java.util.Arrays; //indent:1 exp:0 // no error, invalid (line 3)
import java. //indent:0 exp:0 // no error, valid (line 4)
    util.Arrays; //indent:4 exp:4 // no error, CS says expecting (line 5)
import java. //indent:0 exp:0 // no error, valid (line 6)
                       util.Arrays; //indent:23 exp:4 // no error, invalid (line 7)

D:> java -jar checkstyle-6.12.1-all.jar -c config.xml test.java

Starting audit...
Audit done.

CS reports no errors on the code.
I was expecting CS to give error on the line 3, that says invalid because all imports should start at column 0.
I was expecting CS to give error on the line 7, that says invalid because CS gives an error saying it expected indent "4" if I put it at column 0 (line 5 is example), so I figured it would require exactly "4" for consistency.

@romani romani changed the title from IndentationCheck: incorrect validation for imports to IndentationCheck: incorrect validation for imports and package Nov 18, 2015

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 24, 2016

Member

Default Formatting for IDEs:
Eclipse:

import java.util.Arrays; //indent:1 exp:0
import java. //indent:0 exp:0
        util.Arrays; //indent:4 exp:4
import java. //indent:0 exp:0
        util.Arrays; //indent:23 exp:4

IntelliJ:

import java.util.Arrays; //indent:1 exp:0
import java. //indent:0 exp:0
        util.Arrays; //indent:4 exp:4
import java. //indent:0 exp:0
        util.Arrays; //indent:23 exp:4

NetBeans:

import java.util.Arrays; //indent:1 exp:0
import java. //indent:0 exp:0
        util.Arrays; //indent:4 exp:4
import java. //indent:0 exp:0
        util.Arrays; //indent:23 exp:4

Everyone is in agreement on how imports should be formatted. Only real difference is Eclipse uses tabs, everyone else used spaces.
From original issue, Line 7 is technically correct indentation unless user specifies forceStrictCondition as it is a wrapped line. But right now, even that won't even show a violation.

Member

rnveach commented Apr 24, 2016

Default Formatting for IDEs:
Eclipse:

import java.util.Arrays; //indent:1 exp:0
import java. //indent:0 exp:0
        util.Arrays; //indent:4 exp:4
import java. //indent:0 exp:0
        util.Arrays; //indent:23 exp:4

IntelliJ:

import java.util.Arrays; //indent:1 exp:0
import java. //indent:0 exp:0
        util.Arrays; //indent:4 exp:4
import java. //indent:0 exp:0
        util.Arrays; //indent:23 exp:4

NetBeans:

import java.util.Arrays; //indent:1 exp:0
import java. //indent:0 exp:0
        util.Arrays; //indent:4 exp:4
import java. //indent:0 exp:0
        util.Arrays; //indent:23 exp:4

Everyone is in agreement on how imports should be formatted. Only real difference is Eclipse uses tabs, everyone else used spaces.
From original issue, Line 7 is technically correct indentation unless user specifies forceStrictCondition as it is a wrapped line. But right now, even that won't even show a violation.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 24, 2016

Member

@romani Right now for imports on second line (line 5/7), we use basicOffset for its expected indentation instead of lineWrappingIndentation. Example:


Is this intentional or do we want to switch it to lineWrappingIndentation as this is a wrapped line?

Member

rnveach commented Apr 24, 2016

@romani Right now for imports on second line (line 5/7), we use basicOffset for its expected indentation instead of lineWrappingIndentation. Example:


Is this intentional or do we want to switch it to lineWrappingIndentation as this is a wrapped line?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 3, 2016

Member

we use basicOffset for its expected indentation instead of lineWrappingIndentation

technically import wrapping should use lineWrappingIndentation - as it is pure wrapping.
You can fix this if you want.
But nobody(users) care, as all of them use IDEs formatting that usually single line.

Member

romani commented Jun 3, 2016

we use basicOffset for its expected indentation instead of lineWrappingIndentation

technically import wrapping should use lineWrappingIndentation - as it is pure wrapping.
You can fix this if you want.
But nobody(users) care, as all of them use IDEs formatting that usually single line.

romani added a commit that referenced this issue Jun 3, 2016

@romani romani added the bug label Jun 3, 2016

@romani romani added this to the 7.0 milestone Jun 3, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 3, 2016

Member

fix is merged

Member

romani commented Jun 3, 2016

fix is merged

@romani romani closed this Jun 3, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 3, 2016

Member

You can fix this if you want.

This was already part of the code, so no fix is required.

Member

rnveach commented Jun 3, 2016

You can fix this if you want.

This was already part of the code, so no fix is required.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 3, 2016

Member

good ! thanks a lot.

Member

romani commented Jun 3, 2016

good ! thanks a lot.

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