-
Notifications
You must be signed in to change notification settings - Fork 146
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
Issue #464: Check for trailing commas on enums #722
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
//////////////////////////////////////////////////////////////////////////////// | ||
// checkstyle: Checks Java source code for adherence to a set of rules. | ||
// Copyright (C) 2001-2018 the original author or authors. | ||
// | ||
// This library is free software; you can redistribute it and/or | ||
// modify it under the terms of the GNU Lesser General Public | ||
// License as published by the Free Software Foundation; either | ||
// version 2.1 of the License, or (at your option) any later version. | ||
// | ||
// This library is distributed in the hope that it will be useful, | ||
// but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
// Lesser General Public License for more details. | ||
// | ||
// You should have received a copy of the GNU Lesser General Public | ||
// License along with this library; if not, write to the Free Software | ||
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
package com.github.sevntu.checkstyle.checks.coding; | ||
|
||
import com.puppycrawl.tools.checkstyle.api.AbstractCheck; | ||
import com.puppycrawl.tools.checkstyle.api.DetailAST; | ||
import com.puppycrawl.tools.checkstyle.api.TokenTypes; | ||
|
||
/** | ||
* Checks if enum constant contains an optional trailing comma. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let make it absolutely clear that Check do no not validate all trailing commas, it demand presence of only one comma that is after enum constant list. Please make a link to jdk specification |
||
* | ||
* <p>Rationale: Putting this comma in makes is easier to change the order of the elements or add | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there should be mentioned that Rationale is applicable only for cases when enum is formatted to have enum constant separate on line. Should this Check skip single line enums ? in other case what is a benefit of this Check on code like: |
||
* new elements at the end of the list. This is similar to | ||
* <a href="https://checkstyle.org/config_coding.html#ArrayTrailingComma">ArrayTrailingComma</a>. | ||
* | ||
* <p>The following is a normal enum type declaration: | ||
* <pre> | ||
* enum Type { | ||
* ALPHA, | ||
* BETA, | ||
* GAMMA | ||
* } | ||
* </pre> | ||
* | ||
* <p>However, if you want to append something to the list, you would need to change the line | ||
* containing the last enum constant: | ||
* | ||
* <pre> | ||
* enum Type { | ||
* ALPHA, | ||
* BETA, | ||
* GAMMA, // changed due to the ',' | ||
* DELTA // new line | ||
* } | ||
* </pre> | ||
* | ||
* <p>This check makes sure that also the last enum constant has a trailing comma, which is | ||
* valid according to the Java Spec (see <a | ||
* href="http://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.9.1">Enum Constants</a>) | ||
* | ||
* <pre> | ||
* enum Type { | ||
* ALPHA, | ||
* BETA, | ||
* GAMMA, | ||
* DELTA, // removing this comma will result in a violation with the check activated | ||
* } | ||
* </pre> | ||
* | ||
* <p>However, you could also add a semicolon behind that comma on the same line, which would raise | ||
* a violation | ||
* | ||
* <pre> | ||
* enum Type { | ||
* ALPHA, | ||
* BETA, | ||
* GAMMA, | ||
* DELTA,; // violation | ||
* } | ||
* </pre> | ||
* In this case the semicolon should be removed. However, if there is more in the enum body, the | ||
* semicolon should be placed on a line by itself. | ||
* | ||
* <p>An example of how to configure the check is: | ||
* <pre> | ||
* <module name="EnumTrailingComma"/> | ||
* </pre> | ||
* | ||
* <p>Please note that using this check together with {@code NoWhitespaceBefore} or | ||
* {@code SeparatorWrap} may create conflicts with enums that contain a body: | ||
* this check enforces the semicolon on a separate line while {@code NoWhiteSpaceBefore} does | ||
* not allow the semicolon to be preceded with whitespace and {@code SeparatorWrap} expects the | ||
* semicolon to be on the same line as the last enum constant. | ||
* | ||
* @author <a href="kariem.hussein@gmail.com">Kariem Hussein</a> | ||
*/ | ||
public class EnumTrailingCommaCheck extends AbstractCheck { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it depends probably on your replies above, but looks like, name of Check should be EnumConstantListTrailingCommaCheck , no need change right now, lets wait for clarification on Check behavior and rationale |
||
|
||
/** Key for warning message text in "messages.properties" file. */ | ||
public static final String MSG_KEY = "enum.trailing.comma"; | ||
|
||
@Override | ||
public int[] getDefaultTokens() { | ||
return getAcceptableTokens(); | ||
} | ||
|
||
@Override | ||
public int[] getAcceptableTokens() { | ||
return new int[] {TokenTypes.ENUM_DEF}; | ||
} | ||
|
||
@Override | ||
public int[] getRequiredTokens() { | ||
return getAcceptableTokens(); | ||
} | ||
|
||
@Override | ||
public void visitToken(DetailAST enumDef) { | ||
final DetailAST enumConstBlock = enumDef.findFirstToken(TokenTypes.OBJBLOCK); | ||
|
||
final DetailAST enumConstLeft = enumConstBlock.findFirstToken(TokenTypes.LCURLY); | ||
|
||
DetailAST enumConstEnd = enumConstBlock.findFirstToken(TokenTypes.SEMI); | ||
if (enumConstEnd == null) { | ||
enumConstEnd = enumConstBlock.findFirstToken(TokenTypes.RCURLY); | ||
} | ||
|
||
// Only check, if block is multi-line and there are more than one enum constants | ||
if (enumConstLeft.getLineNo() != enumConstEnd.getLineNo() | ||
&& enumConstBlock.getChildCount(TokenTypes.ENUM_CONSTANT_DEF) > 1) { | ||
|
||
final DetailAST lastSibling = enumConstEnd.getPreviousSibling(); | ||
if (lastSibling.getType() != TokenTypes.COMMA) { | ||
// constant definition does not end in a comma | ||
log(lastSibling, MSG_KEY); | ||
} | ||
else if (enumConstEnd.getType() == TokenTypes.SEMI | ||
&& enumConstEnd.getLineNo() == lastSibling.getLineNo()) { | ||
// constant definition ends in a comma, but is followed by semi on same line | ||
log(enumConstEnd, MSG_KEY); | ||
} | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
//////////////////////////////////////////////////////////////////////////////// | ||
// checkstyle: Checks Java source code for adherence to a set of rules. | ||
// Copyright (C) 2001-2018 the original author or authors. | ||
// | ||
// This library is free software; you can redistribute it and/or | ||
// modify it under the terms of the GNU Lesser General Public | ||
// License as published by the Free Software Foundation; either | ||
// version 2.1 of the License, or (at your option) any later version. | ||
// | ||
// This library is distributed in the hope that it will be useful, | ||
// but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
// Lesser General Public License for more details. | ||
// | ||
// You should have received a copy of the GNU Lesser General Public | ||
// License along with this library; if not, write to the Free Software | ||
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
package com.github.sevntu.checkstyle.checks.coding; | ||
|
||
import static com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck.MSG_KEY; | ||
|
||
import org.junit.Assert; | ||
import org.junit.Test; | ||
|
||
import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport; | ||
import com.puppycrawl.tools.checkstyle.DefaultConfiguration; | ||
|
||
public class EnumTrailingCommaCheckTest extends AbstractModuleTestSupport { | ||
|
||
@Override | ||
protected String getPackageLocation() { | ||
return "com/github/sevntu/checkstyle/checks/coding"; | ||
} | ||
|
||
@Test | ||
public void testDefault() throws Exception { | ||
final DefaultConfiguration checkConfig = | ||
createModuleConfig(EnumTrailingCommaCheck.class); | ||
final String[] expected = { | ||
"14:9: " + getCheckMessage(MSG_KEY), | ||
"20:9: " + getCheckMessage(MSG_KEY), | ||
"27:9: " + getCheckMessage(MSG_KEY), | ||
"33:15: " + getCheckMessage(MSG_KEY), | ||
}; | ||
verify(checkConfig, getPath("InputEnumTrailingCommaCheck.java"), expected); | ||
} | ||
|
||
@Test | ||
public void testTokensNotNull() { | ||
final EnumTrailingCommaCheck check = new EnumTrailingCommaCheck(); | ||
Assert.assertNotNull(check.getAcceptableTokens()); | ||
Assert.assertNotNull(check.getDefaultTokens()); | ||
Assert.assertNotNull(check.getRequiredTokens()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
package com.github.sevntu.checkstyle.checks.coding; | ||
|
||
public interface InputEnumTrailingCommaCheck | ||
{ | ||
enum E1 { | ||
ONE, | ||
TWO, | ||
THREE, | ||
} | ||
|
||
enum E2_1 { | ||
ONE, | ||
TWO, | ||
THREE // violation: no final comma | ||
} | ||
|
||
enum E2_2 { | ||
ONE, | ||
TWO, | ||
THREE // violation: no final comma | ||
; | ||
} | ||
|
||
enum E3 { | ||
ONE, | ||
TWO, | ||
THREE; // violation no final comma, ends in semicolon | ||
} | ||
|
||
enum E4 { | ||
ONE, | ||
TWO, | ||
THREE,; // violation: semicolon on same line as final comma | ||
} | ||
|
||
enum E5 { | ||
ONE, | ||
TWO, | ||
THREE, | ||
; | ||
} | ||
|
||
// enums below are ignored by the check, but were added for completenes | ||
// Please don't remove, they are necessary for full cobertura branch coverage | ||
|
||
// empty | ||
enum E6 {} | ||
|
||
// single enum const, single-line block | ||
enum E7_1 { ONE } | ||
enum E7_2 { ONE; } | ||
enum E7_3 { ONE, } | ||
enum E7_4 { ONE,; } | ||
|
||
// single enum const, multi-line block | ||
enum E8_1 { | ||
ONE | ||
} | ||
enum E8_2 { | ||
ONE; | ||
} | ||
enum E8_3 { | ||
ONE, | ||
} | ||
enum E8_4 { | ||
ONE,; | ||
} | ||
|
||
// check ignores comments | ||
enum E9_1 { | ||
ONE, // this is ok | ||
TWO, | ||
THREE, | ||
} | ||
enum E9_2 { | ||
ONE, | ||
TWO /* this is also ok */ , | ||
THREE, | ||
; | ||
} | ||
|
||
// all ok | ||
enum E_1 { | ||
ONE, TWO | ||
, THREE, FOUR, | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.9.1
please remove "line" , this Check looks like care only about "coma after Enum Constant List" only.
If this is not true, than logic need to be changed.