Indentation: remove requirement right curlies to be first on line #3116

Closed
rnveach opened this Issue Apr 18, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Apr 18, 2016

From Issue #3105 and PR #3113.
comment

}}}); //indent:16 exp:17 warn

CS Violation is: InputInvalidAnonymousClassIndent.java:28: 'method def rcurly' have incorrect indentation level 17, expected level should be one of the following: 12, 16.

CS reports indentation to be 17, when it is in fact 16. I can't temporarly fix the input file to say indent is 17 as then the input file checker flags it as being inconsistent since it sees the indentation as 16.

ioffset was created in the Indentation input files to overcome this issue, that we can report violations on nodes that aren't first in the line.
We need to verify their existence is needed and not another bug of the Indentation check.

When PR is merged, I will point to all marks here and my thoughts on them.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 18, 2016

Member

places where we use ioffset (3 instances)

(1)

public Thread newThread(Runnable runnable) { //indent:12 exp:12
    if (hashCode() == 0) { //indent:16 exp:16
    } else { //indent:16 exp:16
    }}}); //indent:16 ioffset:1 exp:12,16 warn

Violation is for 2nd brace. Ideally it should be at column 12 on the next line.

(2)

System.getProperty("foo"); } //indent:12 ioffset:27 exp:8 warn

for (int i=0; i<10; i++) { //indent:8 exp:8
    System.getProperty("foo"); } //indent:12 ioffset:27 exp:8 warn

Violation is for brace. Ideally it should be at column 8 on the next line.

(3)

System.getProperty("blah"); } //indent:12 ioffset:28 exp:8 warn

if (test) { //indent:8 exp:8
    System.getProperty("blah"); } //indent:12 ioffset:28 exp:8 warn

Same as (2).


All 3 are pretty much the same.

We do not force user linewrap in certain point or single statement per line

The violations right now are basically saying this is what must be done. There is no way around it, as moving the brace backwards will instead report the node that starts the line. I don't consider the violations to be wrong but they definitely violate that rule.

If we truly don't want to force users to break lines, we must remove these and similar violations from being reported. We would have to pass over reporting any node that doesn't start a line. LineWrappingHandler already looks only at nodes that start the line.

If we want the violations to remain, there is no way to remove ioffset without disabling one of our other verifications of the comment, most likely indent because this means indent won't always equal the violation's indentation.

Member

rnveach commented Apr 18, 2016

places where we use ioffset (3 instances)

(1)

public Thread newThread(Runnable runnable) { //indent:12 exp:12
    if (hashCode() == 0) { //indent:16 exp:16
    } else { //indent:16 exp:16
    }}}); //indent:16 ioffset:1 exp:12,16 warn

Violation is for 2nd brace. Ideally it should be at column 12 on the next line.

(2)

System.getProperty("foo"); } //indent:12 ioffset:27 exp:8 warn

for (int i=0; i<10; i++) { //indent:8 exp:8
    System.getProperty("foo"); } //indent:12 ioffset:27 exp:8 warn

Violation is for brace. Ideally it should be at column 8 on the next line.

(3)

System.getProperty("blah"); } //indent:12 ioffset:28 exp:8 warn

if (test) { //indent:8 exp:8
    System.getProperty("blah"); } //indent:12 ioffset:28 exp:8 warn

Same as (2).


All 3 are pretty much the same.

We do not force user linewrap in certain point or single statement per line

The violations right now are basically saying this is what must be done. There is no way around it, as moving the brace backwards will instead report the node that starts the line. I don't consider the violations to be wrong but they definitely violate that rule.

If we truly don't want to force users to break lines, we must remove these and similar violations from being reported. We would have to pass over reporting any node that doesn't start a line. LineWrappingHandler already looks only at nodes that start the line.

If we want the violations to remain, there is no way to remove ioffset without disabling one of our other verifications of the comment, most likely indent because this means indent won't always equal the violation's indentation.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 29, 2016

Member

No violations are expected. Indentation (where non space symbols start the line) is correct.
Indentation Check should not care about any trailing symbols , in other words it is not his business to care about trailing "}" location.

Member

romani commented Apr 29, 2016

No violations are expected. Indentation (where non space symbols start the line) is correct.
Indentation Check should not care about any trailing symbols , in other words it is not his business to care about trailing "}" location.

@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

@romani romani added bug and removed miscellaneous labels Jun 3, 2016

@romani romani changed the title from Inspection and removal of `ioffset` in Indentation input files to Indentation: remove requirement right curlies to be first on line Jun 3, 2016

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