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

LeftCurlyCheck: clarify behavior of 'nlow' option after removal of 'maxLineLength' #3855

Closed
syabru opened this Issue Feb 24, 2017 · 14 comments

Comments

Projects
None yet
5 participants
@syabru

syabru commented Feb 24, 2017

EDIT: only documentation is updated.

/var/tmp $ cat YOUR_FILE.java

public class PaymentAuthorisationRefusedException {
    public PaymentAuthorisationRefusedException(String message, Throwable cause)
    {
        super(message, cause);
    }
}

/var/tmp $ cat config.xml

<?xml version="1.0" encoding="UTF-8"?>
<!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="severity" value="warning"/>
  <module name="TreeWalker">
    <module name="LeftCurly">
      <property name="option" value="nlow"/>
    </module>
    <module name="LineLength">
      <property name="ignorePattern" value="^\s*\* @see .*|import .*$"/>
    </module>
  </module>
</module>

/var/tmp $ java -jar checkstyle-X.XX-all.jar -c config.xml YOUR_FILE.java

Starting audit...
[WARN] /tmp/PaymentAuthorisationRefusedException.java:4:5: '{' at column 5 should be on the previous line. [LeftCurly]
Audit done.

LeftCurly checker defined with nlow (new line on wrap). Default max line length of 80 is used. The statement in question is exactly 80 characters long with left curly brace on new line.

If left curly brace is put on previous line as asked, the following violation triggers:

[WARN] /tmp/PaymentAuthorisationRefusedException.java:3: Line is longer than 80 characters (found 82). [LineLength]

Expectation is that LeftCurly checker takes LineLength (max) into account.


@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 24, 2017

Member

checkstyle 6.9 reports no violation, but 6.10 does.
This looks like it is related to Issue #965 when we deprecated maxLineLength property.

http://checkstyle.sourceforge.net/property_types.html#lcurly

nlow
If the brace will fit on the first line of the statement, then apply eol rule. Otherwise apply the nl rule.

Current code can't follow documentation as how can it know what can and can't fit on a previous line without a definition of how long the lines can be. Current code just looks at whitespace before the { to denote if it should go to the previous line or a new line.

@romani What do you think about this issue? You started original issue.

Member

rnveach commented Feb 24, 2017

checkstyle 6.9 reports no violation, but 6.10 does.
This looks like it is related to Issue #965 when we deprecated maxLineLength property.

http://checkstyle.sourceforge.net/property_types.html#lcurly

nlow
If the brace will fit on the first line of the statement, then apply eol rule. Otherwise apply the nl rule.

Current code can't follow documentation as how can it know what can and can't fit on a previous line without a definition of how long the lines can be. Current code just looks at whitespace before the { to denote if it should go to the previous line or a new line.

@romani What do you think about this issue? You started original issue.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Feb 26, 2017

Contributor

@rnveach

Current implementation of the check requires the following changes to the code:

public class PaymentAuthorisationRefusedException {
    public PaymentAuthorisationRefusedException(String message,
                                                Throwable cause)
    {
        super(message, cause);
    }
}

or

public class PaymentAuthorisationRefusedException {
    public PaymentAuthorisationRefusedException(String message,
        Throwable cause)
    {
        super(message, cause);
    }
}

It looks a bit ugly.

From my point of view, maxLineLength option was introduced to relax the check and to make it less controversial to LineLengthCheck. As you can see from the previous version of the documentation for NLOW:

     * Represents the policy that if the brace will fit on the first line of
     * the statement, taking into account maximum line length, then apply
     * {@code EOL} rule. Otherwise apply the {@code NL}
     * rule. {@code NLOW} is a mnemonic for "new line on wrap".

I remember that we decided to drop the option intentionally, however I believe that we did not take into account the case mentioned above. Note, that RightCurlyCheck does not have such option. I'm not sure whether it is a good idea to return the option. It is not check's responsibility to treat line length, in addition, NLOW was designed to validate other types of code style mistakes. http://checkstyle.sourceforge.net/property_types.html#lcurly#lcurly

Contributor

MEZk commented Feb 26, 2017

@rnveach

Current implementation of the check requires the following changes to the code:

public class PaymentAuthorisationRefusedException {
    public PaymentAuthorisationRefusedException(String message,
                                                Throwable cause)
    {
        super(message, cause);
    }
}

or

public class PaymentAuthorisationRefusedException {
    public PaymentAuthorisationRefusedException(String message,
        Throwable cause)
    {
        super(message, cause);
    }
}

It looks a bit ugly.

From my point of view, maxLineLength option was introduced to relax the check and to make it less controversial to LineLengthCheck. As you can see from the previous version of the documentation for NLOW:

     * Represents the policy that if the brace will fit on the first line of
     * the statement, taking into account maximum line length, then apply
     * {@code EOL} rule. Otherwise apply the {@code NL}
     * rule. {@code NLOW} is a mnemonic for "new line on wrap".

I remember that we decided to drop the option intentionally, however I believe that we did not take into account the case mentioned above. Note, that RightCurlyCheck does not have such option. I'm not sure whether it is a good idea to return the option. It is not check's responsibility to treat line length, in addition, NLOW was designed to validate other types of code style mistakes. http://checkstyle.sourceforge.net/property_types.html#lcurly#lcurly

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 7, 2017

Member

Ok, looks like mode "new line on wrap" is supported by IDEs , so we should do this too
IDEA:
image
image
In Eclipse:
image

BUT this mode have nothing in common with maxLineLength that we are going to deprecate.
Looks like we need to create new issue to create proper support of "nlow" http://checkstyle.sourceforge.net/property_types.html#lcurly that allow location on new line only if there is wrap(multiline) if-statement/for-statement/class-declaration/ .....

@MEZk , @rnveach , @syabru, @Kietzmann please share your vote on this .

Member

romani commented Jul 7, 2017

Ok, looks like mode "new line on wrap" is supported by IDEs , so we should do this too
IDEA:
image
image
In Eclipse:
image

BUT this mode have nothing in common with maxLineLength that we are going to deprecate.
Looks like we need to create new issue to create proper support of "nlow" http://checkstyle.sourceforge.net/property_types.html#lcurly that allow location on new line only if there is wrap(multiline) if-statement/for-statement/class-declaration/ .....

@MEZk , @rnveach , @syabru, @Kietzmann please share your vote on this .

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 7, 2017

Member

this mode have nothing in common with maxLineLength

@romani How will you support nlow ("If the brace will fit on the first line of the statement, then apply eol rule") without maxLineLength?
All your examples show Next line on wrap but a wrap is usually detected/needed if a line goes too far pass the max line length. Changing the max line length in IDEs changes where the wrapping occurs. The 2 seem interconnected to me.

allow location on new line only if there is wrap(multiline) if-statement/for-statement/class-declaration/ .....

User's example shows no clearly defined wrapping happening before {. So you would force the brace to be on the previous line which goes beyound the user's defined max line length?
Your statement seems the exact same as But for a statement spanning multiple lines, Checkstyle will enforce in the documentation. How is what you are proposing any different?
Maybe some examples would help clarify what your proposing that is different?

I don't see why we don't re-enable maxLineLength for proper nlow support unless we remove nlow too.

Member

rnveach commented Jul 7, 2017

this mode have nothing in common with maxLineLength

@romani How will you support nlow ("If the brace will fit on the first line of the statement, then apply eol rule") without maxLineLength?
All your examples show Next line on wrap but a wrap is usually detected/needed if a line goes too far pass the max line length. Changing the max line length in IDEs changes where the wrapping occurs. The 2 seem interconnected to me.

allow location on new line only if there is wrap(multiline) if-statement/for-statement/class-declaration/ .....

User's example shows no clearly defined wrapping happening before {. So you would force the brace to be on the previous line which goes beyound the user's defined max line length?
Your statement seems the exact same as But for a statement spanning multiple lines, Checkstyle will enforce in the documentation. How is what you are proposing any different?
Maybe some examples would help clarify what your proposing that is different?

I don't see why we don't re-enable maxLineLength for proper nlow support unless we remove nlow too.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 10, 2017

Member

How will you support nlow ("If the brace will fit on the first line of the statement, then apply eol rule") without maxLineLength?

We will not support this behavior, we will do the same as IDEs.

All your examples show Next line on wrap but a wrap is usually detected/needed if a line goes too far pass the max line length.

I do not think so, code is changing only if user put EOL symbol and wrap line on second line.

Changing the max line length in IDEs changes where the wrapping occurs.

IDEs format code as they want, we do not force user to do this if user wrap line, we treat it as is.

All your examples show Next line on wrap but a wrap is usually detected/needed if a line goes too far pass the max line length

we will change this rule to follow user EOL symbol and not a line lenght.

User's example shows no clearly defined wrapping happening before {

User's code: public PaymentAuthorisationRefusedException(String message, Throwable cause)
no wrap --> end of line
code should be
public PaymentAuthorisationRefusedException(String message, Throwable cause) {
if LineLengthCheck complains about this line
user need to do code smth like:

public PaymentAuthorisationRefusedException(String message
                                 , Throwable cause) 
{

So you would force the brace to be on the previous line which goes beyound the user's defined max line length?

LieftCurly Check should forget about line length completely - It is not his business.

How is what you are proposing any different?

Looks like, there is confusion of requirements. Issue is not approved. I almost certain to close it invalid, as behavior is correct on reported test case.
We came further and discussing necessity of property that previously affected reported test case.

I don't see why we don't re-enable maxLineLength for proper nlow support unless we remove nlow too.

I do not have open issues on "nlow", probably lack of tests :). But this option should care only about EOL symbols(line wrapping).

Member

romani commented Jul 10, 2017

How will you support nlow ("If the brace will fit on the first line of the statement, then apply eol rule") without maxLineLength?

We will not support this behavior, we will do the same as IDEs.

All your examples show Next line on wrap but a wrap is usually detected/needed if a line goes too far pass the max line length.

I do not think so, code is changing only if user put EOL symbol and wrap line on second line.

Changing the max line length in IDEs changes where the wrapping occurs.

IDEs format code as they want, we do not force user to do this if user wrap line, we treat it as is.

All your examples show Next line on wrap but a wrap is usually detected/needed if a line goes too far pass the max line length

we will change this rule to follow user EOL symbol and not a line lenght.

User's example shows no clearly defined wrapping happening before {

User's code: public PaymentAuthorisationRefusedException(String message, Throwable cause)
no wrap --> end of line
code should be
public PaymentAuthorisationRefusedException(String message, Throwable cause) {
if LineLengthCheck complains about this line
user need to do code smth like:

public PaymentAuthorisationRefusedException(String message
                                 , Throwable cause) 
{

So you would force the brace to be on the previous line which goes beyound the user's defined max line length?

LieftCurly Check should forget about line length completely - It is not his business.

How is what you are proposing any different?

Looks like, there is confusion of requirements. Issue is not approved. I almost certain to close it invalid, as behavior is correct on reported test case.
We came further and discussing necessity of property that previously affected reported test case.

I don't see why we don't re-enable maxLineLength for proper nlow support unless we remove nlow too.

I do not have open issues on "nlow", probably lack of tests :). But this option should care only about EOL symbols(line wrapping).

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 10, 2017

Member

behavior is correct on reported test case.

@romani I still think @syabru or other users would still want this feature as I can understand the need for it as the code change you suggesting seems slightly unnecessary since all can fit on the same line but the brace. Forcing them where they make their line breaks for the non-curlies in this check doesn't seem like that is what this check is about.
I am fine with the way you want to go, but before closing this we should update documentation so it is clearer.
http://checkstyle.sourceforge.net/property_types.html#lcurly

If the brace will fit on the first line

will fit makes it sounds like it is taking the line's length into account.

Member

rnveach commented Jul 10, 2017

behavior is correct on reported test case.

@romani I still think @syabru or other users would still want this feature as I can understand the need for it as the code change you suggesting seems slightly unnecessary since all can fit on the same line but the brace. Forcing them where they make their line breaks for the non-curlies in this check doesn't seem like that is what this check is about.
I am fine with the way you want to go, but before closing this we should update documentation so it is clearer.
http://checkstyle.sourceforge.net/property_types.html#lcurly

If the brace will fit on the first line

will fit makes it sounds like it is taking the line's length into account.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 10, 2017

Member

doc text will be change to:
If the statement/expression/declaration connected to the brace spans multiple lines, than apply eol rule. Otherwise apply the nl rule.

Member

romani commented Jul 10, 2017

doc text will be change to:
If the statement/expression/declaration connected to the brace spans multiple lines, than apply eol rule. Otherwise apply the nl rule.

@romani romani changed the title from LeftCurlyCheck : violation reported for non wrapped line to LeftCurlyCheck: clarify behavior of 'nlow' option after removal of 'maxLineLength' Jul 10, 2017

romani added a commit that referenced this issue Jul 10, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 10, 2017

Member

fix is merged.

Member

romani commented Jul 10, 2017

fix is merged.

@romani romani closed this Jul 10, 2017

@romani romani added this to the 8.1 milestone Jul 10, 2017

@syabru

This comment has been minimized.

Show comment
Hide comment
@syabru

syabru Jul 17, 2017

I was hoping for NLOW support as we had it in Checkstyle 6.9 and how it is implemented by IDE's and its auto-formatting tools. Actually, there was a reason for having the maxLineLength property with this rule.

code should be
public PaymentAuthorisationRefusedException(String message, Throwable cause) {
if LineLengthCheck complains about this line
user need to do code smth like:

public PaymentAuthorisationRefusedException(String message
                                 , Throwable cause) 
{

This formatting looks messy to me and is also not how NLOW auto-formatting in IDE's works.

Changing the documentation to a somewhat obscure definition of NLOW doesn't quite help and I wouldn't consider it as a fix.

syabru commented Jul 17, 2017

I was hoping for NLOW support as we had it in Checkstyle 6.9 and how it is implemented by IDE's and its auto-formatting tools. Actually, there was a reason for having the maxLineLength property with this rule.

code should be
public PaymentAuthorisationRefusedException(String message, Throwable cause) {
if LineLengthCheck complains about this line
user need to do code smth like:

public PaymentAuthorisationRefusedException(String message
                                 , Throwable cause) 
{

This formatting looks messy to me and is also not how NLOW auto-formatting in IDE's works.

Changing the documentation to a somewhat obscure definition of NLOW doesn't quite help and I wouldn't consider it as a fix.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 17, 2017

Member

@syabru , thanks for raising this again.

I was hoping for NLOW support as we had it in Checkstyle 6.9

You need to share very very good reasons to let us change our mind.

This formatting looks messy to me and is also not how NLOW auto-formatting in IDE's works.

lets create new issue and clearly define differences between IDEs and current nlow. As this option is introduced to match IDEs we need to work some how close to IDEs behavior.

Member

romani commented Jul 17, 2017

@syabru , thanks for raising this again.

I was hoping for NLOW support as we had it in Checkstyle 6.9

You need to share very very good reasons to let us change our mind.

This formatting looks messy to me and is also not how NLOW auto-formatting in IDE's works.

lets create new issue and clearly define differences between IDEs and current nlow. As this option is introduced to match IDEs we need to work some how close to IDEs behavior.

@syabru

This comment has been minimized.

Show comment
Hide comment
@syabru

syabru Jul 20, 2017

You need to share very very good reasons to let us change our mind.

AFAICS LeftCurly maxLineLength attribute was set to deprecated and then finally removed due to a lack of understanding for what this attribute is actually needed for (see #965). This happened despite a fully valid explanation from @MEZk (see #965). My suggestion is to revert the changes deprecating and removing this attribute and we are all done.

syabru commented Jul 20, 2017

You need to share very very good reasons to let us change our mind.

AFAICS LeftCurly maxLineLength attribute was set to deprecated and then finally removed due to a lack of understanding for what this attribute is actually needed for (see #965). This happened despite a fully valid explanation from @MEZk (see #965). My suggestion is to revert the changes deprecating and removing this attribute and we are all done.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 20, 2017

Member

Please prove a reason of this property, I do not see a reason now.

Member

romani commented Jul 20, 2017

Please prove a reason of this property, I do not see a reason now.

@errdos

This comment has been minimized.

Show comment
Hide comment
@errdos

errdos Oct 24, 2017

hi,
can you please fix JavaDoc as well?
http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/checks/blocks/LeftCurlyCheck.html
here it seems that it still references maxLineLength

<module name="LeftCurly"> <property name="option" value="nlow"/> <property name="maxLineLength" value="120"/> < /module>
although that i can see there is no actual maxLineLength parameter in the Class

void | setIgnoreEnums(boolean ignoreEnums)Sets whether check should ignore enums when left curly brace policy is EOL.
void | setOption(String optionStr)Set the option to enforce.
void | visitToken(DetailAST ast)Called to process a token.

errdos commented Oct 24, 2017

hi,
can you please fix JavaDoc as well?
http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/checks/blocks/LeftCurlyCheck.html
here it seems that it still references maxLineLength

<module name="LeftCurly"> <property name="option" value="nlow"/> <property name="maxLineLength" value="120"/> < /module>
although that i can see there is no actual maxLineLength parameter in the Class

void | setIgnoreEnums(boolean ignoreEnums)Sets whether check should ignore enums when left curly brace policy is EOL.
void | setOption(String optionStr)Set the option to enforce.
void | visitToken(DetailAST ast)Called to process a token.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 25, 2017

Member

@errdos , issue is closed, please create new issue, and please be welcome with PR if you know how to fix javadoc

Member

romani commented Oct 25, 2017

@errdos , issue is closed, please create new issue, and please be welcome with PR if you know how to fix javadoc

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