Support more tokens in WhitespaceAfter check #3333

Closed
vanniktech opened this Issue Jun 30, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@vanniktech

vanniktech commented Jun 30, 2016

Assigned to @MaksimP­

Right now it only supports COMMA, SEMI, TYPECAST. I'd love to have support for LITERAL_IF that for instance if( will be marked as a checkstyle error because it should be if (.

Also some other would come in handy:

  • LITERAL_IF
  • LITERAL_WHILE
  • LITERAL_FOR
  • LITERAL_DO
  • DO_WHILE
  • LITERAL_NEW
  • LITERAL_ELSE

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@romani romani added the approved label Jul 1, 2016

@eric-milles

This comment has been minimized.

Show comment
Hide comment
@eric-milles

eric-milles Jul 6, 2016

Currently this is supported using WhitespaceAround. Do you have a reason to have "if" not preceded by a space?

        <module name="WhitespaceAround">
            <message key="ws.notPreceded" value="Keyword ''{0}'' should be preceded by a space" />
            <message key="ws.notFollowed" value="Keyword ''{0}'' should be followed by a space" />
            <property name="tokens" value="LITERAL_ASSERT,LITERAL_CATCH,LITERAL_ELSE,LITERAL_FINALLY,LITERAL_FOR,LITERAL_IF,LITERAL_RETURN,LITERAL_SWITCH,LITERAL_SYNCHRONIZED,LITERAL_TRY,LITERAL_WHILE,DO_WHILE" />
        </module>

Currently this is supported using WhitespaceAround. Do you have a reason to have "if" not preceded by a space?

        <module name="WhitespaceAround">
            <message key="ws.notPreceded" value="Keyword ''{0}'' should be preceded by a space" />
            <message key="ws.notFollowed" value="Keyword ''{0}'' should be followed by a space" />
            <property name="tokens" value="LITERAL_ASSERT,LITERAL_CATCH,LITERAL_ELSE,LITERAL_FINALLY,LITERAL_FOR,LITERAL_IF,LITERAL_RETURN,LITERAL_SWITCH,LITERAL_SYNCHRONIZED,LITERAL_TRY,LITERAL_WHILE,DO_WHILE" />
        </module>
@vanniktech

This comment has been minimized.

Show comment
Hide comment
@vanniktech

vanniktech Jul 6, 2016

Uh nice you're right. Thanks a lot! Do you also happen to know if there's a check that flags the following:

    }

}

as it should be

    }
}

Uh nice you're right. Thanks a lot! Do you also happen to know if there's a check that flags the following:

    }

}

as it should be

    }
}
@eric-milles

This comment has been minimized.

Show comment
Hide comment
@eric-milles

eric-milles Jul 6, 2016

I don't think there is a specific check. The way that I have devised is to use multi-line regex checks. In our case, we have CRLF as the endline sequence, which is leveraged by the latter two of the checks below.

Note: If you use "if (...) {" style followed by a blank line for extra spacing, the first check will not be desirable. We use braces on their own line most of the time.

    <module name="RegexpMultiline">
        <property name="message" value="Blank line at start of block should be removed" />
        <property name="format" value="(?&lt;=(?&lt;!^beans )\{\s{0,99}$)^$" />
        <property name="fileExtensions" value="groovy,java" />
    </module>
    <module name="RegexpMultiline">
        <property name="message" value="Blank line at end of block should be removed" />
        <property name="format" value="(?&lt;!\{\s{0,99}$)^$(?=^\s{0,99}\})" />
        <property name="fileExtensions" value="groovy,java" />
    </module>
    <module name="RegexpMultiline">
        <property name="message" value="Blank line before start of block should be removed" />
        <property name="format" value="(?&lt;=[!-:&lt;-~] {0,48}?\r\n {0,48}?)\r\n *(\{|else)" />
        <property name="fileExtensions" value="groovy,java" />
    </module>
    <module name="RegexpMultiline">
        <property name="message" value="Extra blank line(s) should be removed" />
        <property name="format" value="^(?:(?:\r\n){2,}|\r\n$)" />
        <property name="fileExtensions" value="groovy,html,htm,java,jj,jjt,vm,xml,xsl" />
    </module>

I don't think there is a specific check. The way that I have devised is to use multi-line regex checks. In our case, we have CRLF as the endline sequence, which is leveraged by the latter two of the checks below.

Note: If you use "if (...) {" style followed by a blank line for extra spacing, the first check will not be desirable. We use braces on their own line most of the time.

    <module name="RegexpMultiline">
        <property name="message" value="Blank line at start of block should be removed" />
        <property name="format" value="(?&lt;=(?&lt;!^beans )\{\s{0,99}$)^$" />
        <property name="fileExtensions" value="groovy,java" />
    </module>
    <module name="RegexpMultiline">
        <property name="message" value="Blank line at end of block should be removed" />
        <property name="format" value="(?&lt;!\{\s{0,99}$)^$(?=^\s{0,99}\})" />
        <property name="fileExtensions" value="groovy,java" />
    </module>
    <module name="RegexpMultiline">
        <property name="message" value="Blank line before start of block should be removed" />
        <property name="format" value="(?&lt;=[!-:&lt;-~] {0,48}?\r\n {0,48}?)\r\n *(\{|else)" />
        <property name="fileExtensions" value="groovy,java" />
    </module>
    <module name="RegexpMultiline">
        <property name="message" value="Extra blank line(s) should be removed" />
        <property name="format" value="^(?:(?:\r\n){2,}|\r\n$)" />
        <property name="fileExtensions" value="groovy,html,htm,java,jj,jjt,vm,xml,xsl" />
    </module>
@vanniktech

This comment has been minimized.

Show comment
Hide comment
@vanniktech

vanniktech Jul 7, 2016

@eric-milles thanks man. Very much appreciated. @romani since the functionality I asked for is supported by a different check I'll leave the decision up to you whether this should be implemented or not.

@eric-milles thanks man. Very much appreciated. @romani since the functionality I asked for is supported by a different check I'll leave the decision up to you whether this should be implemented or not.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 1, 2016

Member

@romani Can you confirm that this is still an approved issue?
I see nothing wrong with adding the tokens, but then why doesn't WhitespaceAfter have more things from WhitespaceAround for space required after token.

I don't think LITERAL_NEW should be added as it is impossible to write code without a space after new. The Java compiler thinks it is a method name and not an instantiation clause. Ex: newString().

Member

rnveach commented Aug 1, 2016

@romani Can you confirm that this is still an approved issue?
I see nothing wrong with adding the tokens, but then why doesn't WhitespaceAfter have more things from WhitespaceAround for space required after token.

I don't think LITERAL_NEW should be added as it is impossible to write code without a space after new. The Java compiler thinks it is a method name and not an instantiation clause. Ex: newString().

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 2, 2016

Member

Can you confirm that this is still an approved issue?

I am ok to close it, if original author do not need it any more.

I see nothing wrong with adding the tokens,

I am still ok to have this issue, as Checks should be independent and user should be able customize them to match his needs.

why doesn't WhitespaceAfter have more things from WhitespaceAround for space required after token

have no idea, all Checks are independent, and extended/developed by user requests ....

I don't think LITERAL_NEW should be added as it is impossible to write code without a space after

yes, this literal should not be present. We do not need nonsense. If users find example where it could be used - we will extend it later.

Member

romani commented Aug 2, 2016

Can you confirm that this is still an approved issue?

I am ok to close it, if original author do not need it any more.

I see nothing wrong with adding the tokens,

I am still ok to have this issue, as Checks should be independent and user should be able customize them to match his needs.

why doesn't WhitespaceAfter have more things from WhitespaceAround for space required after token

have no idea, all Checks are independent, and extended/developed by user requests ....

I don't think LITERAL_NEW should be added as it is impossible to write code without a space after

yes, this literal should not be present. We do not need nonsense. If users find example where it could be used - we will extend it later.

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 3, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 3, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 6, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 7, 2016

Member

token DO_WHILE was added.

Member

romani commented Nov 7, 2016

token DO_WHILE was added.

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 14, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 14, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 15, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 15, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 17, 2016

Issue #3333: done
Issue #3333:  done

Issue #3333: done

Issue #3333: done

Issue #3333: done

Issue #3333: done

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 17, 2016

Issue #3333: add new tokens in WhitespaceAfterCheck
Issue #3333:  done

Issue #3333: done

Issue #3333: done

Issue #3333: done

Issue #3333: done

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 17, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 17, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 19, 2016

Issue #3333: add new literals in WhiteSpaceAfterCheck
Issue #3333:  fix code decoration

Issue #3333: add token DO_WHILE

Issue #3333: change object argument

Issue #3333: fix error

Issue #3333: fix error @link tag

Issue #3333: fix error url

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 19, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 19, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 19, 2016

Member

New tokens could be default as it generally acceptable style.
It is not conflicting with google style http://checkstyle.sourceforge.net/reports/google-java-style.html#s4.6.2-horizontal-whitespace , even this check is not in use at google_checks.xml .

Member

romani commented Nov 19, 2016

New tokens could be default as it generally acceptable style.
It is not conflicting with google style http://checkstyle.sourceforge.net/reports/google-java-style.html#s4.6.2-horizontal-whitespace , even this check is not in use at google_checks.xml .

@vanniktech

This comment has been minimized.

Show comment
Hide comment
@vanniktech

vanniktech Nov 19, 2016

I'm in favor of making them the default.

I'm in favor of making them the default.

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 28, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 28, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 29, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 29, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 29, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 29, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 30, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Nov 30, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Dec 8, 2016

MaksimP added a commit to MaksimP/checkstyle that referenced this issue Dec 9, 2016

romani added a commit that referenced this issue Dec 9, 2016

@romani romani added the new feature label Dec 9, 2016

@romani romani added this to the 7.4 milestone Dec 9, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 9, 2016

Member

fix is merged.

Member

romani commented Dec 9, 2016

fix is merged.

@romani romani closed this Dec 9, 2016

@eric-milles

This comment has been minimized.

Show comment
Hide comment
@eric-milles

eric-milles Dec 9, 2016

Will this generate multiple violations if both WhitespaceAround and WhitespaceAfter are used?

Will this generate multiple violations if both WhitespaceAround and WhitespaceAfter are used?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 9, 2016

Member

Check is responsible for his violations. if you have few Checks that do kind of the same you will have multiple violations.

Member

romani commented Dec 9, 2016

Check is responsible for his violations. if you have few Checks that do kind of the same you will have multiple violations.

@eric-milles

This comment has been minimized.

Show comment
Hide comment
@eric-milles

eric-milles Dec 9, 2016

So, as I indicated, WhatespaceAround already supports all those tokens. Now you're making them default in WhitespaceAfter when they should probably be optional since they are default in WhitespaceAround. http://checkstyle.sourceforge.net/config_whitespace.html#WhitespaceAround

Isn't the more common case that you do not allow symbol characters directly adjacent to keywords like if? And so the WhitespaceAround has the more sensible defaults.

So, as I indicated, WhatespaceAround already supports all those tokens. Now you're making them default in WhitespaceAfter when they should probably be optional since they are default in WhitespaceAround. http://checkstyle.sourceforge.net/config_whitespace.html#WhitespaceAround

Isn't the more common case that you do not allow symbol characters directly adjacent to keywords like if? And so the WhitespaceAround has the more sensible defaults.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 9, 2016

Member

we very very rare consider dependencies between Checks. All of them works like it is only one Check in the configuration.
It is user responsibility to adjust config to receive less duplicates or behavior he likes.
By the way two violation on the same code is even better, kind of "second opinion".

Member

romani commented Dec 9, 2016

we very very rare consider dependencies between Checks. All of them works like it is only one Check in the configuration.
It is user responsibility to adjust config to receive less duplicates or behavior he likes.
By the way two violation on the same code is even better, kind of "second opinion".

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