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

reevaluate tokens in google config for SeparatorWrapCheck #3752

Closed
rnveach opened this Issue Jan 24, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Jan 24, 2017

Taken from PR #3743,
When reviewing the tokens of SeparatorWrapCheck, we would like more time to review the following tokens before adding them.

http://checkstyle.sourceforge.net/config_whitespace.html#SeparatorWrap

"RBRACK", "AT", "ELLIPSIS", "SEMI", "ARRAY_DECLARATOR"

Right now we ignore usage of these tokens in google config validator:

        GOOGLE_TOKENS_IN_CONFIG_TO_IGNORE.put("SeparatorWrap", Stream.of(
                // state of configuration until
                // https://github.com/checkstyle/checkstyle/issues/3752
                "RBRACK", "AT", "ELLIPSIS", "SEMI", "ARRAY_DECLARATOR",

We need to review https://google.github.io/styleguide/javaguide.html and make a decision:

  • to use this tokens (remove from GOOGLE_TOKENS_IN_CONFIG_TO_IGNORE, update google_checks.xml, update google's ITs to have inputs with such code)
  • do NOT use (keep as is and update ignore comment at AllChecksTest ).
    If google style is not clear on this use cases - we could open issue on style guide to clarify.
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 26, 2017

Member

http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.8.5-annotations

Annotations applying to a class, method or constructor ... each annotation is listed on a line of its own
Annotations applying to a field ... multiple annotations (possibly parameterized) may be listed on the same line

We can't differentiate between field and non-field in this Check. So we can't add AT token.

http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.8.3-arrays

I'm not sure anything specifies location of ARRAY_DECLARATOR (the [ token of []). Examples talk about after [] mostly.

@romani I can't find any documentation on RBRACK, ELLIPSIS, SEMI, in the document anywhere.

Member

rnveach commented Jan 26, 2017

http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.8.5-annotations

Annotations applying to a class, method or constructor ... each annotation is listed on a line of its own
Annotations applying to a field ... multiple annotations (possibly parameterized) may be listed on the same line

We can't differentiate between field and non-field in this Check. So we can't add AT token.

http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.8.3-arrays

I'm not sure anything specifies location of ARRAY_DECLARATOR (the [ token of []). Examples talk about after [] mostly.

@romani I can't find any documentation on RBRACK, ELLIPSIS, SEMI, in the document anywhere.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 26, 2017

Member

We can't differentiate between field and non-field in this Check. So we can't add AT token.

confirmed. please update ignore comment.

I'm not sure anything specifies location of ARRAY_DECLARATOR (the [ token of []). Examples talk about after [] mostly.

http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ARRAY_DECLARATOR
http://checkstyle.sourceforge.net/property_types.html#wrapOp , usage as:

new int
[] {0, 1, 2, 3}

is so weird that it is hard to imagine that smb will do this, but we can recheck this :) .

ELLIPSIS

it should be eol . ... at the begging of line is weird.

SEMI

I think semi-colon should be eol.
What for smb can write a code like following ?

doSmth()
;

RBRACK

http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#RBRACK
I think it should be eol .
arr[1] = 1;

--EXPR -> EXPR [4:11]
   `--ASSIGN -> = [4:11]
       |--INDEX_OP -> [ [4:7]
       |   |--IDENT -> arr [4:4]
       |   |--EXPR -> EXPR [4:8]
       |   |   `--NUM_INT -> 1 [4:8]
       |   `--RBRACK -> ] [4:9]
       `--NUM_INT -> 1 [4:13]

how do you like following code ?

arr[1
] = 1
Member

romani commented Jan 26, 2017

We can't differentiate between field and non-field in this Check. So we can't add AT token.

confirmed. please update ignore comment.

I'm not sure anything specifies location of ARRAY_DECLARATOR (the [ token of []). Examples talk about after [] mostly.

http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ARRAY_DECLARATOR
http://checkstyle.sourceforge.net/property_types.html#wrapOp , usage as:

new int
[] {0, 1, 2, 3}

is so weird that it is hard to imagine that smb will do this, but we can recheck this :) .

ELLIPSIS

it should be eol . ... at the begging of line is weird.

SEMI

I think semi-colon should be eol.
What for smb can write a code like following ?

doSmth()
;

RBRACK

http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#RBRACK
I think it should be eol .
arr[1] = 1;

--EXPR -> EXPR [4:11]
   `--ASSIGN -> = [4:11]
       |--INDEX_OP -> [ [4:7]
       |   |--IDENT -> arr [4:4]
       |   |--EXPR -> EXPR [4:8]
       |   |   `--NUM_INT -> 1 [4:8]
       |   `--RBRACK -> ] [4:9]
       `--NUM_INT -> 1 [4:13]

how do you like following code ?

arr[1
] = 1
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 26, 2017

Member

@romani I was trying to go by the rules alone in the documentation for specific allow/disallow. I agree allowing forms that the check would prevent look weird and shouldn't be allowed, but google doesn't specify one way or the other unless I am missing it. If someone comes to complain, it is better we have something from the document to point them to for needing more examples or confirmation.

prefer to break at a higher syntactic level

Do you know what this means?

The primary goal for line wrapping is to have clear code

Since the examples you gave made the code not easy to read, we'll use this as the excuse to add the other tokens.

Member

rnveach commented Jan 26, 2017

@romani I was trying to go by the rules alone in the documentation for specific allow/disallow. I agree allowing forms that the check would prevent look weird and shouldn't be allowed, but google doesn't specify one way or the other unless I am missing it. If someone comes to complain, it is better we have something from the document to point them to for needing more examples or confirmation.

prefer to break at a higher syntactic level

Do you know what this means?

The primary goal for line wrapping is to have clear code

Since the examples you gave made the code not easy to read, we'll use this as the excuse to add the other tokens.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 26, 2017

Member

but google doesn't specify one way or the other unless I am missing it.

it is ok, as it is impossible to specify all. Even we come to this point after few years of detailed reading of google style. If think our team is the only team who read this document with a lot of attention.
Google still in process of polishing of this document.
Lets create issue on google style to clarify.

higher syntactic level

we investigated this term in scope of indentation
https://google.github.io/styleguide/javaguide.html#s4.5.2-line-wrapping-indent
two continuation lines use the same indentation level if and only if they begin with syntactically parallel elements.
It refers to syntax of language. As ASTs are different in all parsers and in all human minds.
We should use abstract syntax that is described at jdk specification - https://docs.oracle.com/javase/specs/jls/se8/html/jls-19.html .
Example:

VariableDeclarator:
VariableDeclaratorId [= VariableInitializer]

so wrap should be at/after =.

Member

romani commented Jan 26, 2017

but google doesn't specify one way or the other unless I am missing it.

it is ok, as it is impossible to specify all. Even we come to this point after few years of detailed reading of google style. If think our team is the only team who read this document with a lot of attention.
Google still in process of polishing of this document.
Lets create issue on google style to clarify.

higher syntactic level

we investigated this term in scope of indentation
https://google.github.io/styleguide/javaguide.html#s4.5.2-line-wrapping-indent
two continuation lines use the same indentation level if and only if they begin with syntactically parallel elements.
It refers to syntax of language. As ASTs are different in all parsers and in all human minds.
We should use abstract syntax that is described at jdk specification - https://docs.oracle.com/javase/specs/jls/se8/html/jls-19.html .
Example:

VariableDeclarator:
VariableDeclaratorId [= VariableInitializer]

so wrap should be at/after =.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 28, 2017

Member

SEMI

lets keep ignoring this token, as ; could be a formating element in enums that are arranged in column, and in complicated sentences(builders) to keep ; on separate line and allow ease insertion of new line.

enum {
 V1,
 V2,
 ;
}
 buider.setSmth()
    .setSmth2()
    .setSmth3()
 ;
Member

romani commented Jan 28, 2017

SEMI

lets keep ignoring this token, as ; could be a formating element in enums that are arranged in column, and in complicated sentences(builders) to keep ; on separate line and allow ease insertion of new line.

enum {
 V1,
 V2,
 ;
}
 buider.setSmth()
    .setSmth2()
    .setSmth3()
 ;
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 5, 2017

Member

@rnveach , do you plan to continue ?

Member

romani commented Mar 5, 2017

@rnveach , do you plan to continue ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 5, 2017

Member

@romani Not at this point, if you wish to assign this to someone to get it done faster than go ahead.

Member

rnveach commented Mar 5, 2017

@romani Not at this point, if you wish to assign this to someone to get it done faster than go ahead.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 1, 2017

Member

@kazachka , it would be awesome if you help us to resolve this issue, this is last issue that blocks us before new checkstyle8 release.

Member

romani commented May 1, 2017

@kazachka , it would be awesome if you help us to resolve this issue, this is last issue that blocks us before new checkstyle8 release.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 22, 2017

Member

@kazachka ,

Summary of what to do:

  1. ARRAY_DECLARATOR should be "eol" and please create issue on Google style guide
  2. ELIPSIS should be "eol" and please create issue on Google style guide
  3. SEMI keep in ignore and put a comment "location could be any, as people like to keep it on separate line for enum values, and ...."
  4. RBRACK keep in ignore and put a comment "location could be any, as people can draw any layout with of arrays indexes ...."
    Example:
int value = arr[
                    getInex1() + some number + 6 + 2389629 + getShift()
                 ][
                    getInex1() + some number + 5 + 2389620 + getShift1()
                 ]

Please do not forget to update IT(test code and inputs) for such cases.

Member

romani commented Jun 22, 2017

@kazachka ,

Summary of what to do:

  1. ARRAY_DECLARATOR should be "eol" and please create issue on Google style guide
  2. ELIPSIS should be "eol" and please create issue on Google style guide
  3. SEMI keep in ignore and put a comment "location could be any, as people like to keep it on separate line for enum values, and ...."
  4. RBRACK keep in ignore and put a comment "location could be any, as people can draw any layout with of arrays indexes ...."
    Example:
int value = arr[
                    getInex1() + some number + 6 + 2389629 + getShift()
                 ][
                    getInex1() + some number + 5 + 2389620 + getShift1()
                 ]

Please do not forget to update IT(test code and inputs) for such cases.

kazachka added a commit to kazachka/checkstyle that referenced this issue Jul 1, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Jul 4, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Jul 4, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Jul 5, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Jul 8, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Jul 9, 2017

rnveach added a commit that referenced this issue Jul 9, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 9, 2017

Member

Fix was merged

Member

rnveach commented Jul 9, 2017

Fix was merged

@rnveach rnveach closed this Jul 9, 2017

@rnveach rnveach added this to the 8.1 milestone Jul 9, 2017

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