Issue #464: Check for trailing commas on enums #709
Conversation
|
Please let me know what has to be done to fix the builds. I will try to get around this within the next few days. Please also let me know what needs to be done in addition to merge this pull request. |
|
The following are the CI failures.
Check must be added to https://raw.githubusercontent.com/checkstyle/contribution/master/checkstyle-tester/checks-sevntu-error.xml .
I will have to see what is wrong. Checkstyle regression will require changes in main checkstyle project based on violations from this check. |
|
@kariem ,
Please provide update to our code in separate commit, it better to make it first commit in this PR, as it unlikely to change. Such commit culd be merged even before main fix to ease merge of new Check. |
|
Hmm, a lot time passes from original discussion, originally reported at checkstyle/checkstyle#3161. We are not in moratorium any more, so if @kariem ok it is better to contribute to main project directly. But we can finish contribution to sevntu first, whatever is easier. @rnveach , please review main issue and approve it. |
This was fixed. CI now shows results of regresion. Main checkstyle project has 27 violations from this new check. |
|
Wercker failure is strange, I cleaned cache and relaunched build. |
|
wercker passed, so CI validation is working correctly now. |
|
@romani Sorry, I do not understand what needs to be done. If I have to change the checkstyle code, I will have to do this in another PR on checkstyle/checkstyle, right? What's your recommendation? I think you guys can better assess what is the best way forward. |
|
I suggest to finish merge to sevntu, as a lot already done and we are close to merge state. It is better to merge, this PR sooner. |
|
@romani Then it is your call on what to do in this PR. CI will never be green here without changes to main repo and external file. |
|
Let's merge new Check to sevntu first. Failure in checkstyle-regression
Please send PR to https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/checks-sevntu-error.xml |
I have created a pull request in checkstyle/checkstyle (referenced above). However, there seem to be conflicts with other checks. Is it necessary to enable all checks? I think it's quite obvious that some checks do not play well with others.
Does that mean that I should add |
I didn't predicted a conflict , so it is not obvious. Need to be warned in documentation as minimum.
Yes, please do, sorry for confusion. |
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
from
b54be53
to
5917f4f
Compare
I was talking about the general implication of requirements for code formatting. It is obvious to me that some checks have conflicting requirements. In this special case, I did not know that there were conflicts until I have seen them. However, I assumed that you were aware of checks that are already in place in CI.
Done: 5917f4f. Is this sufficient for you? |
|
I will add to the checkstyle/contribution/checkstyle-tester/checks-sevntu-error.xml as soon as we have agreed on a solution to the conflicting checks. I hope this also makes sense to you. |
Good. Please squash commits in one.
Please add it with ignore severity. Looks like Check is too conflicting now. |
|
@kariem To disable regression for your new check, please add it to the ignore list at: |
The check can still be added, as is, to this configuration. The configuration already sets severity to ignore for all checks. See https://github.com/checkstyle/contribution/blob/789055b817de65290d31c125f5b7cb256679197f/checkstyle-tester/checks-sevntu-error.xml#L15 . |
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
2 times, most recently
from
b43332b
to
b2b1aed
Compare
|
I have now disabled the check for regression as instructed by rnveach and squashed all commits into one. I have created a separate issue and PR on checkstyle/contribution for inclusion of EnumTrailingCommaAndSemicolonCheck. @rnveach @romani Please let me know, if there is anything else I can do now. |
|
I need some time to review, I am afk till Tuesday . Side note is that logic should be "similar" to http://checkstyle.sourceforge.net/config_coding.html#ArrayTrailingComma Looks like trailing |
| <description>%EnumTrailingCommaCheck.desc</description> | ||
|
|
||
| <message-key key="enum.trailing.comma"/> | ||
| <message-key key="enum.trailing.comma.semi"/> |
| } | ||
| } | ||
|
|
||
| private boolean isIllegalTokenAfterComma(DetailAST lastComma, DetailAST nextAst) { |
There was a problem hiding this comment.
please explain why this method is not isSmthAfterCommaOnSameLine with no special processing of SEMI.
I think it should be checking for any sibling on same line.
There was a problem hiding this comment.
According to the Java Language Specification on Enum Constants the only allowed symbols are
- Other enum constants, separated by a comma
- Enum body declarations, starting with a semicolon
If this method does not allow any sibling on the same line, this would also include traditional comments (starting with TokenType.BLOCK_COMMENT_BEGIN) and end-of-line comments (TokenType.SINGLE_LINE_COMMENT). Both, however, should be allowed. Am I misunderstanding something?
There was a problem hiding this comment.
your Check is not comments aware, so in AST there will be no comments at all.
example of how comments could appear in AST for Check - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/CommentsIndentationCheck.java#L104
so if you are not comment aware check - I see no reason in extra validation of tokens.
If you want your Check to be aware of comments , you can do this, and describe in documentation of Check that comments are ignored (so it is the same as working on AST without comments).
please make a UT case in input file with comment to make it clear that you do not violate such lines.
If this resolve an issue, I would appreciate your feedback on how to improve our documentation on comments in AST.
There was a problem hiding this comment.
@romani Just checked the implementation again. Good to know that we do not need to check for comments, so the explicit check on symbols seems to be redundant.
However, after looking at this again, I still think the current version is very clear on what is done: we have two problem cases:
-
The last comma is followed by an enum constant: this means that there is no more comma after that enum constant, because we have already checked for the last comma
-
The last comma is followed by a semicolon on the same line.
This is the implementation
private boolean isIllegalTokenAfterComma(DetailAST lastComma, DetailAST nextAst) {
boolean illegalToken = false;
final int nextAstType = nextAst.getType();
if (nextAstType == TokenTypes.SEMI) {
if (nextAst.getLineNo() == lastComma.getLineNo()) {
// semi on the same line as last comma
illegalToken = true;
}
}
else if (nextAstType == TokenTypes.ENUM_CONSTANT_DEF) {
// next enum constant on the same line after comma
illegalToken = true;
}
return illegalToken;
}I have now tried different options and could not find a clearer solution, so I do not really understand what you are asking for when you do not want the "extra validatoin" of tokens. We will always have to check for the semicolon or the right curly bracket to see whether we are at the end of the enum constant block.
Also, this method could be a lot less verbose so I have just updated it. See be8be03
private boolean isIllegalTokenAfterComma(DetailAST lastComma, DetailAST nextAst) {
final int nextAstType = nextAst.getType();
// semi on the same line as last comma, or followed by enum constant
return (nextAstType == TokenTypes.SEMI && nextAst.getLineNo() == lastComma.getLineNo())
|| nextAstType == TokenTypes.ENUM_CONSTANT_DEF;
}Does that work for you?
There was a problem hiding this comment.
The last comma is followed by an enum constant: this means that there is no more comma after that enum constant, because we have already checked for the last comma
The last comma is followed by a semicolon on the same line.
Fact that I do not understand is why we care about "last comma" ? all we should care is last enum element and symbol that is following it. If following symbol/element is not comma - it is violation. If after comma there is smth on same line - it is violation.
In other words - recheck two elements of AST after last enum constant.
There was a problem hiding this comment.
I have rewritten that whole block. It's now similar to the ArrayTrailingCommaCheck, with some additional comments:
enumConstEnd can either be SEMI or 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);
}
}With this, we can remove the other helper methods getLastComma and isIllegalTokenAfterComma.
Does that work for you?
There was a problem hiding this comment.
@romani I haven't directly answered the question, but I think the current solution is better than I had previously suggested. Let me quickly answer the question
Fact that I do not understand is why we care about "last comma"?
We only care about the "last comma". This is the point of the check. Hence the name EnumTrailingCommaCheck. Intermediary commas are required as per language specification. Only the last, the trailing comma, is relevant for this check.
sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java
Show resolved
Hide resolved
| .asList("ReturnCountExtendedCheck"); | ||
| .asList( | ||
| "EnumTrailingCommaCheck", | ||
| "ReturnCountExtendedCheck" |
There was a problem hiding this comment.
please add comment above "// initial ",
we will update it ones your PR is merged.
@rnveach , do you remember a reason ?
There was a problem hiding this comment.
@romani The reason was probably because it wasn't enabled in main repo as this is regression script.
See checkstyle/checkstyle#3496 where it was enforced and checkstyle/checkstyle#3628 where it was first added. The last issue is a direct link to this script, so the check was never enabled at the time the regression was created.
This line can probably be removed.
There was a problem hiding this comment.
See my comment on the above review note: I have only added the comment to the line for EnumTrailingCommaCheck. I don't think touching the code regarding ReturnCountExtendedCheck is in the scope of this PR.
There was a problem hiding this comment.
ok, look ok now , I will update reason for second issue , ones we merge you.
|
CI is failing.
|
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
from
bc2730a
to
4a30256
Compare
|
Squashed all changes and pushed again: 4a30256 Please let me know what you think needs to be done in addition to merge this pull request. It would be very helpful, if you could write all up in one go. The back and forth is very time consuming for all parties involved. |
Sorry, it is all my bad, I agree, but I do review of all PRs and requests and issues, it is hard fro me to spend whole my time only on one PR and make it merged with minimal amount of interactions. |
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
2 times, most recently
from
f5906c1
to
1b87398
Compare
|
Squashed all changes and pushed again: 1b87398 Please let me know what you think needs to be done in addition to merge this pull request. |
|
@kariem there is still a checkstyle violation, be sure to run
|
@rnveach I did run this and did not get any errors. I re-ran it again after an
Thank you for the information. This relates to this line: // semi on the same line as last comma, or followed by enum constant
return (nextAstType == TokenTypes.SEMI && nextAst.getLineNo() == lastComma.getLineNo())
|| nextAstType == TokenTypes.ENUM_CONSTANT_DEF;As I cannot reproduce this and do not want to push changes until it works, could you tell me, how I can correct this line? Should I blow it up again as we had this as indicated in #709 (comment) ? Thank you for your help. |
|
Sorry, for sevntu to check itself the command is The violation just wants you to swap the contents between the |
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
from
1b87398
to
dabe711
Compare
|
Thank you for the clarification! Just updated this part.
…On Mon, Oct 22, 2018 at 4:38 AM rnveach ***@***.***> wrote:
Sorry, for sevntu to check itself the command is mvn -e verify
-Pno-validations,selftesting.
The violation just wants you to swap the contents between the ||. So
instead of a || b, it wants b || a.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#709 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAcCPEyNRc4HK6J829ZPoc3QBfkmZLkXks5unS-5gaJpZM4VhtF6>
.
|
There was a problem hiding this comment.
reply to old discussion - dabe711#r227181567
Please make in input cases like:
One, Two // is it a violation ?
, Three, Four
;
And
One, Two
;
I just want to be sure we are know what we are validating, and clear define this in description for users
extra minor item to improve:
| enum E5 { | ||
| ONE, | ||
| TWO, | ||
| THREE, |
There was a problem hiding this comment.
please make a test case with trailing comments (// some comment) after "ONE" and after "TREE" to make it clear that Check do not see comments at all.
There was a problem hiding this comment.
Added a separate section at the end for both, traditional and end-of-line comments.
// check ignores comments
enum E9_1 {
ONE, // this is ok
TWO,
THREE,
}
enum E9_2 {
ONE,
TWO /* this is also ok */ ,
THREE,
;
}There was a problem hiding this comment.
@romani I missed to address the first part of this request.
Changes:
- Separate section with your exampe, slightly adapted
The trailing comma after
// all ok enum E_1 { ONE, TWO , THREE, FOUR, }
FOURis important here - Included another check on the missing enum, if followed by a semicolon on the next line
enum E2_2 { ONE, TWO, THREE // violation ; }
I have also added more information to the // violation comments to explain the respective reasons.
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
from
dabe711
to
3a827f1
Compare
|
@romani From my perspective this is done. From what I can see, all the side-conversations that have not been closed or resolved are either resolved or outdated and redundant. If there is anything else for me to do here, please write one comment with everything that needs to be done. This way I can do everything you want and have this pull request closed. The current process is very annoying and time consuming. |
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
from
3a827f1
to
57428dc
Compare
kariem
force-pushed
the
464-enum-trailing-comma-and-semicolon
branch
from
57428dc
to
b86c563
Compare
|
@kariem , sorry that it takes so long but idea of Check somewhere is broken that is wh awe sendup with long discussion. I still see inconsistency in behavior and what Check try to address.
Behavior: We need to clearly define what we are validating. Se you in new PR. |
|
@romani Please describe what the inconsistency is for you and I will try to address this. The idea of the check follows ArrayTrailingComma. If you do not want what is done in ArrayTrailingComma, we have a different discussion. I do not think a new PR helps here, as you might have noticed, this is PR number 4 and I have addressed all your requests in the previous PRs and in this PR. See #722 for a follow-up. |
|
Javadoc first sentence just tell about training comma, but in fact Check care only about comma after last element. This need to be fixed in javadoc if this is intentional. This check does not demand each element of enum to be on separate line. If that is intentional we just need to make it clear in javadoc, to make it clear that this Check address only particular case. |
Trailing comma is the comma after the last element. Either there is a trailing comma, or there is none. This check is only about trailing commas. Please see similar rules in JavaScript, TSLint, or the often linked ArrayTrailingComma in Checkstyle (also referred to in the javadoc).
Of course, this does not demand anything else. This also ignores single-line enum constant definitions, because there is no benefit. This does not check comments, as they are not really relevant for what we are doing. Please also go through your discussion around the check for trailing commas in arrays in checkstyle/checkstyle#1509 I have tried to use terms commonly known in Checkstyle and also known in other closely related languages. I did not invent anything new here and for me the description is very clear. If you think there is a problem with the terms used, please suggest an improvement. If you think something is not clear enough, please suggest a clarification. |
|
Sorry that I ma demanding, but I do this for a reason. There are hundreds of Checks and their goals a not very clear that result in many bugs reported on Checks and had to keep project under control. Please see my comments in new PR. |
Addresses #464, supersedes #465 and #474.
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.