Issue #464: Check for trailing commas on enums #722
Conversation
| @@ -183,6 +183,9 @@ EitherLogOrThrowCheck.loggingMethodNames = Logging method names separated with c | |||
| WhitespaceBeforeArrayInitializerCheck.name = Whitespace Before Array Initializer | |||
| WhitespaceBeforeArrayInitializerCheck.desc = This checks enforces whitespace before array initializer. | |||
|
|
|||
| EnumTrailingCommaCheck.name = Enum Trailing Comma | |||
| EnumTrailingCommaCheck.desc = This checks enforces a trailing comma at the end of a line in an enum constant definition. | |||
There was a problem hiding this comment.
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.
| import com.puppycrawl.tools.checkstyle.api.TokenTypes; | ||
|
|
||
| /** | ||
| * Checks if enum constant contains an optional trailing comma. |
There was a problem hiding this comment.
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
https://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.9.1
| /** | ||
| * Checks if enum constant contains an optional trailing comma. | ||
| * | ||
| * <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.
there should be mentioned that Rationale is applicable only for cases when enum is formatted to have enum constant separate on line.
if Enum is single line - there is no benefit of this check.
Should this Check skip single line enums ? in other case what is a benefit of this Check on code like:
enum COLOR { RED, BLACK } ?
It might be ok to demand come here too, but Rationale need to be updated. I am not sure how to update it. Please suggest something, as you are author of this idea, you might know more use cases.
| * | ||
| * @author <a href="kariem.hussein@gmail.com">Kariem Hussein</a> | ||
| */ | ||
| public class EnumTrailingCommaCheck extends AbstractCheck { |
There was a problem hiding this comment.
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
|
@kariem , ping. |
|
It is really sad that such a huge amount of work did not come to end. we lost connection to Author of this code, I am closing PR .... if author or any other person ready to continue, we can reopen PR or start it from scratch. As I see from comments, only minor items (naming/doc) are left to be done. |
|
@romani I think you are right with the proposed changes. I hope you can understand that after the huge amount of work invested in all the previous PRs (#465, #474, #709), it is difficult for me to make yet another change and then wait calmly for more requests for changes from your side. Could you please check, if there is anything else left? If I implement the changes requested in #722 (review) how many more changes can I expect to have to go through to have this PR merged? |
I know this for sure.
PR is reopened. I will review.
I am not sure. Check need to pass CI, regression testing report (slightly manual), 2 human reviews. |
|
@romani You said, you would review and then you close it? Did I misunderstand you? I would like to finish the work here. |
|
I do not see changes, please push them and reply to each review point as done or ... |
Addresses #464, supersedes #465, #474, and #709
This Check assures that the last enum constant definition in a multi-line enum ends in a comma:
Usage examples are in the Javadoc of
EnumTrailingCommaCheck. This check follows the concept of ArrayTrailingComma, hence the similar name.It also checks that the "trailing" comma after the last enum constant definition is not followed by a semicolon, because this would miss the point of the check: clean diff and easy manipulation.