reevaluate tokens in google config for NeedBracesCheck #3753

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

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Jan 24, 2017

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

LAMBDA

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 25, 2017

Member

NeedBracesCheck will require braces always when the lambdas are used.

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

Braces are used with if, else, for, do and while statements

It doesn't mention lambda, so it is not requesting them all the time.
http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.1.2-blocks-k-r-style is only mentioning if the braces are added.

Going even further, the newest style guide has an example of a lambda with no braces.
https://google.github.io/styleguide/javaguide.html#s4.5.1-line-wrapping-where-to-break

@romani It seems to me there is nothing that requests lambda have braces all the time. So the configuration shouldn't change.
We just need to update the reason in the test.

Member

rnveach commented Jan 25, 2017

NeedBracesCheck will require braces always when the lambdas are used.

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

Braces are used with if, else, for, do and while statements

It doesn't mention lambda, so it is not requesting them all the time.
http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.1.2-blocks-k-r-style is only mentioning if the braces are added.

Going even further, the newest style guide has an example of a lambda with no braces.
https://google.github.io/styleguide/javaguide.html#s4.5.1-line-wrapping-where-to-break

@romani It seems to me there is nothing that requests lambda have braces all the time. So the configuration shouldn't change.
We just need to update the reason in the test.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 26, 2017

Member

yes, comment need to be updated

from new guide:

A line is never broken adjacent to the arrow in a lambda, except that a break may come immediately after the arrow if the body of the lambda consists of a single unbraced expression.

It is requirements. can Check cover it ?

Member

romani commented Jan 26, 2017

yes, comment need to be updated

from new guide:

A line is never broken adjacent to the arrow in a lambda, except that a break may come immediately after the arrow if the body of the lambda consists of a single unbraced expression.

It is requirements. can Check cover it ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 26, 2017

Member

can Check cover it ?

@romani
NeedBraces doesn't deal with line breaks and the requirement is unbraced.
unbraced also throws out LeftCurlyCheck.
Even if SeparatorWrap or OperatorWrap could handle the lambda token, which they can't, they can't handle the extra requirement by default (if the body of the lambda consists of a single unbraced expression). The checks just look at the token itself for the most part.

Member

rnveach commented Jan 26, 2017

can Check cover it ?

@romani
NeedBraces doesn't deal with line breaks and the requirement is unbraced.
unbraced also throws out LeftCurlyCheck.
Even if SeparatorWrap or OperatorWrap could handle the lambda token, which they can't, they can't handle the extra requirement by default (if the body of the lambda consists of a single unbraced expression). The checks just look at the token itself for the most part.

rnveach added a commit to rnveach/checkstyle that referenced this issue Jan 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jan 28, 2017

romani added a commit that referenced this issue Jan 28, 2017

@romani romani added this to the 7.5 milestone Jan 28, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 28, 2017

Member

Fix is merged

Member

romani commented Jan 28, 2017

Fix is merged

@romani romani closed this Jan 28, 2017

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