Check for trailing commas on enums #465
Conversation
|
from CI: should be fixed. |
This checks that the last enum constant definition in a multi-line enum ends in a comma:
enum E1 {
ONE,
TWO,
THREE,
}
Addresses sevntu-checkstyle#464
kariem
force-pushed
the
enum-trailing-commas
branch
from
e9e47c4
to
af7482f
Compare
|
I thought the trivial changes did not ask for a separate commit, so I pushed an amend to the original. Please tell me if there is anything else that needs to be done. |
|
@kariem , please extend your javadoc with examples of usages, as it will be the only source for documentation on how to use your Check. You have two violation messages so you definitely need to explain all cases (right now you have nothing about second validating case). Please put reference to issue number to git commit message for future investigations.
that is not good, please throw IllegalStateException there , most likely you did this code due tu demand for 100% coverage, so please make a test to launch a Check on bad tree structure to make this exception happen.
as your Check does not have any properties, please update input file to have "//violation" trailing comment on line there violation is going to happen |
Provide examples for the check and extend the rationale behind it. Addresses sevntu-checkstyle#464
The default branch on the previous switch did not do anything and should not have raised an exception, which would be invalid for single-line enums. Addresses sevntu-checkstyle#464
|
@romani, thank you very much for looking over the code!
Done: 67870e9 There is some not-so-nice formatting in the line with the reference to the Java Language Specification. The checkstyle rule for javadoc should be changed: it limits line length to 100 characters, which prevents adding URLs. I could not deactivate a single line in Javadoc with
It's there on the first commit and all the subsequence commits, always on the last line. If you mean, putting the issue number on the first (summary) line, I don't think it's a good idea (1, 2). However, I can put it on line 3 (after the blank line), if you prefer that.
Yes, I agree: this is not good. However, the Done: 29b4c08
Done: 2211b55 |
|
Please squash all commits in one to ease review of final state of the code |
|
@romani have you considered creating option here to prohibit such trailing commas if someone considers them as unnecessary code? |
|
not yet, I do not see whole design of Check yet, we are in a progress .... |
|
@romani @mkordas the design is similar to ArrayTrailingComma. In the initial commit (af7482f), even the javadoc and logic were similar, but changes to this were requested, as can be seen in the history of this PR. |
@romani This would change the history and the whole discussion we've had in this PR. We would lose quite a bit of information. I don't see the point of this. For the review of all changes or the final state of the code, use the files tab. After reviewing, you should be able to just merge all commits everything squashed into a single commit, following the instructions here. |
|
What's missing here? Any indication on when we can get this done? |
|
Sorry for delay, i will try to process your code as soon as i have time. Please pin me in a week if no feedback provided |
|
It's ok - don't worry. Just wanted to make sure nothing is missing on my side. Thank you very much! |
|
@kariem ,
history of discussion is minor in comparison to history of git and well written issue description.
yes I know this, but this is error prone in my experience, if you disagree please treat it as rule of this project. |
"valid" , we use such words for marking violations in code, please change your word to "usual" or smth else. I have some concerns about validation for "semicolon" , it is not clear from Check name that it will do that validation. .... let me think about this. |
I understand the point you are making: the git history is the "only truth". GitHub has a good way to only have one commit (per pull request) in the commit history without altering the discussion in the pull request. Please follow the instructions here to do a Squash and Merge as soon as you think this is ready. I have already linked this previously. Alternatively, as soon as there are no changes from your side, I can open another pull request with only one commit, if you prefer that.
It's in every commit on the last line - do you want it at the start of the commit message in the summary? I am asking, because it somehow contradicts the importance of the history in git you have mentioned. If the reference to the issue is this prominent, the remaining summary text has to be shortened to 12 characters less than without that prefix.
I have just adapted the wording in e238658. You can also use line comments to make the process of finding the issues you want to correct easier for me.
The idea of the check is to have easy manipulation of enums and clean diffs. If you have a semicolon on the line of the last enum constant, it won't work. I believe this check has to include this. |
|
@kariem ,
thanks a lot, but all of that is already adjusted and I use it when it necessary.
no need in new PR, please do git changes and just do "git push --force" to your topic branch , PR will be refreshed and show your latest code.
Sorry my bad, please do single line commit message. Prefix with ref to issue that I mentioned is required.
So we need to change a name of it.
In brief description there should be short description of whole validation rule.
please make an example in javadoc of what is right format of enum. |
I understand and I will squash as soon as everything is accepted, so that you can merge with minimum mistakes. Until then, to view all changes, please click on the Files Changed Tab. I hope you can see that it does not make sense for me to go through this on every commit. I perfectly understand that, as you say, you do not care about our history of discussion in the PR. In the end everything will just be as you prefer. Please also respect my time and efforts as I respect yours.
Ok, I understand that. I will certainly do so on the squash.
The check cares about the comma and it should be the last element on the line. If there is a semicolon after the comma, the comma is not a trailing comma. Whatever the benefit of the check, we are checking the trailing comma, not a clean diff or an easy manipulation of the enum constants. Naming the check after potential benefits might be very difficult in the future and people might want to include indentation or even newline checks (both reasonable on a check named clean diff), which should not be part of this check. On the other side, I strongly believe in consistent naming. The name is chosen based on the same functionality in ArrayTrailingComma (I have already referred to this in a previous comment). There cannot be any semicolons in an array, so every comparison at the level of "The ArrayTrailingComma check does not check for semicolons" is not helpful. I want this change to eventually get to the Checkstyle project (see checkstyle/checkstyle#3161). We would then need to rename I will follow suite with items 3 (brief description of the whole validation rule) and 4 (example on semicolon placement) as soon as we agree on the above. It does not make sense to do something here the way I think is correct, if you don't agree. |
please squash it now.
for you may be, but I need this to simplify my workflow, I have to do number of code review per day in project, I need help from you, to accept your code quicker.
sorry, it was my typo.
hmm, let me review one more time in comparison with other Checks. Consistent naming is good but self-descriptive naming is better. |
|
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ArrayTrailingCommaCheck.java this Check does not care about ";" , so for consistency of names and behavior please remove validation for ";" . If you need this validation , please create just another Check. |
|
@kariem , is there any other thoughts , disagreements ? |
|
@kariem , is there a chance to continue work on PR. |
|
please squash all commits and update initial description and let me review (summary of all point) : request for changes and implementation one more time. |
|
@romani Thank you for the quick reply. However, I don't think you are in a position to estimate how much time it takes me to do this: you have no information about how much time it took me to get here. On the other side, it seems you ignore the fact that everything we have discussed up to this point is in this pull request and it is not necessary for anyone to remember anything. I will provide one commit and a clear description as soon as I can. |
I see a problem that issue/pr is not done/merged. I see long history of conversations and points. We could keep it like this(in state of nobody want to read and ..... ) or we could start discussion from the begging with all our evidences of sense summed up. If you do not have time to continue, lets close PR , somebody else will find time to make it happen. |
|
Good idea! I also thought, it might be better for everyone involved to create a new PR. I'll try to come around this later tonight. |
This Check assures that the last enum constant definition in a multi-line enum ends in a comma:
enum E1 {
ONE,
TWO,
THREE,
}
Original PR: sevntu-checkstyle#465
2016-07-29 -- 2016-08-26
|
@romani I've opened #474 and will close this. I have also checked Allow edits from maintainers in case there is something trivial you see needs changes. I will also close this here, because everything is in one commit on the other PR. |
This Check assures that the last enum constant definition in a multi-line enum ends in a comma:
enum E1 {
ONE,
TWO,
THREE,
}
Original PR: sevntu-checkstyle#465
2016-07-29 -- 2016-08-26
This Check assures that the last enum constant definition in a multi-line enum ends in a comma:
enum E1 {
ONE,
TWO,
THREE,
}
Original PR: sevntu-checkstyle#465
2016-07-29 -- 2016-08-26
This checks that the last enum constant definition in a multi-line enum ends in a comma:
Addresses #464