Issue #464: Check for trailing commas on enums

[kariem]

This Check assures that the last enum constant definition in a multi-line enum ends in a comma:

enum E1 {
    ONE,
    TWO,
    THREE,
}

Adresses #464, supersedes #465.

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.

[coveralls]

Coverage increased (+0.01%) to 98.228% when pulling 2cbf931 on kariem:464-enum-trailing-comma into fe257b8 on sevntu-checkstyle:master.

[rnveach]

@romani Ping.

[romani]

@kariem , sorry for delay, I was too busy with main project (checkstyle).

ok, so implementation did change from previous PR.
I still believe that name of the Check is not correct - EnumTrailingCommaCheck.
Example:

enum Type {
     ALPHA,
     BETA,
     GAMMA,
     DELTA,; // violation
}

Name of the Check EnumTrailingCommaCheck does not tell user that it is also validating semicolon presence around trailing comma. So mentioning consistency in name is not accurate to my mind.
I do not like Checks that do too much (or smth that could be done as another Check), because such approach cause problems with support. User start to complain about extra features and ask to let skip extra validations by properties, ..... that results in complicated logic in Check and nobody want to support such Checks.

I could merge this Check to sevntu (this is whole purpose of this project), but I will be against this Check movement to main project (checkstyle).

So, it is time to make a decision, fortunately now we have @rnveach for second opinion. @rnveach , please let us know your opinion.

Options:

1. merge this Check as is and release it as sevntu-checkstyle Check (but I will be against simple move of this Check to checkstyle project).
2. Rename the Check to be clear in name.

[rnveach]

@romani
If it is just a simple name change, I think we have no reason we can't do it now. It shouldn't take but a few minutes.

No one will remember this conversation later, so if we can, let's get everything done now.

[romani]

@rnveach , whole conflict is about the name vs functionality. @kariem , am I right ?

@kariem , looks like I am not alone with idea that name does not match functionality. So ...
It is your choice what to do: keep name or rename.
Please let me know your choice and lets proceed with merge.

[romani]

@kariem , ping.

[kariem]

Sorry for the delay. I just do not see why something like this can take so long. I had to take some time to write this together. It's important to me to solve this and I am quite sure quick shots, one-liners and minimal effort won't make us complete this soon.

It seems @romani, you are in a lot of different places, and it's difficult to follow through. I can understand that. Let me wrap up here quickly.

• It is correct that the name does not tell the user explicitly that a semicolon cannot be on the same line behind the enum constant.
• It is not correct that the implementation changed from the previous PR (Check for trailing commas on enums #465). Please check that again.
• I agree that a check should not do anything the user does not expect.
• I agree with @rnveach that a simple name change should not hold us up.

I think the name is good, that's why I chose it. I've summed up my thoughts previously.

In my personal opinion, I expect the check for ArrayTrailingComma do the same thing as a similar check on enums. It is clear to me that such a check has to do a little bit more than a check on arrays, because there can be more "content" after the enum constants (i.e. the body).

In JavaScript, there is one check in ESLint for this: comma-dangle. It's used for everything: arrays, objects, imports, exports, and functions. In Java, we cannot use the same mechanism, but we should at least for the sake of a user make it simple.

Please choose a name. I can throw in my opinion, if requested, or start a doodle on the mailing list or anywhere you like. I just won't suggest a different name, because I strongly believe that the current name is good, and the suggested alternatives are worse.

Please assess the arguments yourself or include anyone you want to include. Here is what I see against and for the current name:

➖ Documentation: A check should only do what it says it does.
➕ Discoverability: A user should find a check easily.
➕ Association: Seeing the check for arrays should make it clear what to use for enum constants.
➕ Consistency: Similar configuration across checks

Please accept the changes we have now. We can always change everything later, if we see better options. I can do the necessary work.

[romani]

@kariem ,

Please choose a name.
vs
Please accept the changes we have now.

I am not sure about your decision. Please state it clearly.

Please choose a name.

My proposal: EnumTrailingCommaAndSemicolonCheck

[kariem]

@romani: sorry for the ambiguities. I will make this more explicit:

1. Please accept the changes we have now.
2. Please decide on whether you want to change the name. If you want to change the name, please do so.

[rnveach]

@kariem Please rebase over latest master and fix any failing tests or code style errors.
We are in the process of massively revamping of sevntu and your changes will fail travis if merged now.

Check must be added to config. Check must be added to sonar and eclipse-cs. New code styles are being enforced on code.

If you do make these changes, I also ask that you please rename check to EnumTrailingCommaAndSemicolonCheck as @romani has suggested.

[kariem]

@rnveach I have now rebased on master and renamed class and test class as requested.

It's great that you have new code style errors, but clean up your code first, before you introduce it on new changes. This is not very professional. Is there a reason why you guys are blocking this pull request? Is this on purpose?

The class EnumTrailingCommaAndSemicolonCheck has the same header as FinalizeImplementationCheck, but the first one fails on CI. Do you have IDE settings, especially for the imports documented somewhere? This would greatly help

[rnveach]

@kariem
That's what these changes are, we are cleaning up our own code and enforcing more of the same things we require on main project.
None of these code style checks are new. They just weren't being run against sevntu and on the test folder. They have been running against main project for a while now.

The class EnumTrailingCommaAndSemicolonCheck has the same header as FinalizeImplementationCheck, but the first one fails on CI.

https://travis-ci.org/sevntu-checkstyle/sevntu.checkstyle/builds/181098451#L649
Failures are against 'EnumTrailingCommaAndSemicolonCheckTest' (Please notice Test on the end). This is not your main Check class.
Looking at your files, https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/474/files#diff-e63994a65725711cd3489e5a60b940cf , you are missing the header. Main project requires these same styles.

Do you have IDE settings, especially for the imports documented somewhere?

Yes.
http://checkstyle.sourceforge.net/beginning_development.html
http://checkstyle.sourceforge.net/eclipse.html#Organize_Imports
Usually violation from check is clear enough to make change that way. Statics should be first. Next is java package. Then org package`. Then everything else. All should be in alphabetical order.

[kariem]

@rnveach Thank you very for the quick reply. I did not expect this.

Problems related to imports seem to be resolved now. I use IntelliJ IDEA and the instructions for the imports are here - just a reference.

One more request: Could you please elaborate on the other additional requests (config, sonar, and eclipse-cs) in your previous comment? Is this documented somewhere? I've seen the CI fails for two cases:

• Check referenced in config file sevntu-checks.xml
• Module configuration in sonar

It also fails on the commit message on the initial commit! This currently looks like this

Issue #464: Check for trailing commas on enums

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: #465

2016-07-29 -- 2016-08-26

See e5f09e5

Why would you do that? The commit comment describes what has changed and nobody expects to go to the issue tracker for every minor detail. How can the commit describe an issue like this in a single line?

[rnveach]

additional requests (config, sonar, and eclipse-cs) in your previous comment

The documentation is: https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/What-files-need-to-be-changed-or-created-in-repository-for-new-Check-implementation
But it is not very detailed as main project.

Changes are expected in: eclipsecs-sevntu-plugin\src\com\github\sevntu\checkstyle\checks\coding (both files)
And: sevntu-checkstyle-sonar-plugin\src\main\resources\com\github\sevntu\checkstyle\sonar\checkstyle-extensions.xml

You can look at existing checks and follow the same suite. JUnit will guide you through most of it if you make a mistake.
It is just adding your check, your name/description, properties, and expected error messages in different XML formats.

com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaAndSemicolon is not referenced in sevntu-checks.xml

Copy and paste as is. No Check on end.

Why would you do that? The commit comment describes what has changed and nobody expects to go to the issue tracker for every minor detail. How can the commit describe an issue like this in a single line?

Written in original issue by romani:

in my experience, if you try to describe smth in multi-line (a lot of text) you should create issue and describe all that without limiting yourself (describe with wide details and examples and links) to commit size. People subconsciously try to be short in commit message, so most of their attempts to be descriptive and short in the same time - always end-up as partly unclear text with missed context.
No ability to extend messages (with extra details/context) after it was merged to master.
Another case is when engineer try to put in the same commit too much unrelated stuff, that should be split into few commits.

[coveralls]

Coverage increased (+0.009%) to 98.348% when pulling 2d71cef on kariem:464-enum-trailing-comma into c03e3d4 on sevntu-checkstyle:master.

[coveralls]

Coverage increased (+0.009%) to 98.348% when pulling 99f7748 on kariem:464-enum-trailing-comma into c03e3d4 on sevntu-checkstyle:master.

[kariem]

@rnveach Thank you for the quick answers. I've now updated as requested. All the configuration files have been updated.

However, the build fails, because some files don't adhere to the newly introduced check ref. That check is not useful: we cannot test that the source of sevntu adheres to all possible checks that can be configured in sevntu. There may be conflicts between different checks.

[coveralls]

Coverage increased (+0.006%) to 98.777% when pulling 81ddb7b on kariem:464-enum-trailing-comma into 0c7b77a on sevntu-checkstyle:master.

[kariem]

@rnveach to conclude this, I have added one more commit to resolve the last error reported by CI: cdc74e8. Seems to be ok now. Either this should be configurable, or I missed something.

Anyways, I think the current configuration is not reasonable. All these artificially strict rules discourage contributions.

Please indicate, if there is something else to be done. From what I see, I have followed all your instructions and requests.

[romani]

Anyways, I think the current configuration is not reasonable. All these artificially strict rules discourage contributions.

yes, it is discourage contribution for some time, but attract new contributors in future as code is clean, and welcoming other contributor come to your code. Nobody wants to go to badly written code, especially for free (opensource). If you still do not understand .... just keep it as our rules.
So please follow the rules. Unfortunately process is not well documented and defined but are in progress .... . And this project is not our job, so there are delays .....

[romani]

@kariem ,

please do more items:

1. review https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/What-files-need-to-be-changed-or-created-in-repository-for-new-Check-implementation looks like last two points are not done, Please do.
   If you have questions - please ask.

Please squash all functional commits (introduction of Check) in one commit. All changes in sevntu code that was caused by new Check in unrelated files, please do in separate commit of the same PR. There should be clear where is new functionality and where is affect of it.

I did not found any testing reports of new Check.
We do this even on bug fixes, and new Check should definitely have pass it too.
Here is a tool that will help you to do this - https://github.com/checkstyle/contribution/tree/master/checkstyle-tester , please read README.txt , should be very easy, if not please ask questions we will guide you.
There should be two testing:
a) stress testing for Exception detection (your Check, on a lot of sources, no exceptions are expected)
b) functional testing (your Check on reasonable amount of sources, to review all violations to be functionally correct)
All reports should be shared with us to review.
Here is groovy script (https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/launch.groovy) that do the same as shell script.

[kariem]

@romani please don't ask me to "please follow the rules", if this is what I am doing. You should strongly consider improving your workflows or your documentation. I am suggesting improvements. If you cannot accept suggestions, it will be difficult for you to find additional contributions. I am not saying I know better, but I have to point out that this process does not seem to work.

This is unreadable. How can I provide a patch?

Checking out the project does not work because of the file names:

$ git clone https://github.com/sevntu-checkstyle/sevntu.checkstyle.wiki.git
Cloning into 'sevntu.checkstyle.wiki'...
remote: Counting objects: 1156, done.
remote: Total 1156 (delta 0), reused 0 (delta 0), pack-reused 1156
Receiving objects: 100% (1156/1156), 673.10 KiB | 726.00 KiB/s, done.
Resolving deltas: 100% (701/701), done.
Checking connectivity... done.
error: unable to create file Development-workflow-with-Git:-Fork,-Branching,-Commits,-and-Pull-Request.textile (Invalid argument)
fatal: unable to checkout working tree
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry the checkout with 'git checkout -f HEAD'

On a side note, please look at #465 to see what we have discussed already and just imagine what a motivated contributor has to do to get his work included in this project. Please note that everything was considered completed already -- with your confirmation -- and you keep adding additional tasks to a very simple change. This is not professional. This is not your day job and I assume you don't get money for this, however, if you say you are responsible, it is your job.

review [What-files-need-to-be-changed-or-created-in-repository-for-new-Check-implementation] looks like last two points are not done, Please do.
If you have questions - please ask.

The last two items are

1. Check needs to be applied to checkstyle
2. Change the default configuration on the website at sevntu-checkstyle-default-configuration.xml

Ad (1): I've opened #3163 in May and I will create a pull request on Checkstyle as soon as this is pull request is accepted. We have to take one step after the other. How can I update Checkstyle, if you haven't even accepted any of the changes here?

Ad (2): Please explain how I can change the default configuration on the website.

Please squash all functional commits (introduction of Check) in one commit. All changes in sevntu code that was caused by new Check in unrelated files, please do in separate commit of the same PR. There should be clear where is new functionality and where is affect of it.

I won't work around everything again. I need one ok from you or @rnveach on the current state and be sure that nothing else comes my way. How can I continue, if we keep playing ping pong? This is not fun for me. I also have a day job and a lot of other things on my list, and I cannot keep changing the commits just because you don't want to confirm my contributions.

I did not found any testing reports of new Check. [use checkstyle-tester]

I have just run against the current my_check.xml and one test with only this check as module. Versions used:

• Checkstyle tester: 95d7ef, current master
• Checkstyle: b61daf, current master
• Sevntu (checks and maven plugin): cdc74e8 current version in this PR, based on c03e3d4, current master

The results from the target directory and the configuration used are here: checkstyle-tester-results-enum-trailing-comma-20161208.zip

Please suggest how we can move forward.

[romani]

How can I provide a patch?

unfortunately github does not allow PR to wiki :( . This is still undone for many years.
if you want to improve it it is better to do this by code, I will merge them manually, I did this before - http://roman-ivanov.blogspot.com/2013/11/how-to-merge-github-wiki-changes-from.html
clone works fine on my side (Ubuntu 14.04)

$ git clone https://github.com/sevntu-checkstyle/sevntu.checkstyle.wiki.git
Cloning into 'sevntu.checkstyle.wiki'...
remote: Counting objects: 1156, done.
remote: Total 1156 (delta 0), reused 0 (delta 0), pack-reused 1156
Receiving objects: 100% (1156/1156), 673.10 KiB | 0 bytes/s, done.
Resolving deltas: 100% (701/701), done.
Checking connectivity... done.

please provide more details from your side.

and you keep adding additional tasks to a very simple change.

Sorry, I am not 100% focused on this project, I could loose a context.
Unfortunately where is no simple way to write a document to express what is required for contributor to do. PRs are different, I make decision case by case, base on my long experience in this project.
Just follow my guidance I never demand useless stuff.
I you do not like rules, just do fork and be independent. As you wont to host your code - follow recommendations.

How can I update Checkstyle, if you haven't even accepted any of the changes here?

easy. PR with fixes in code and update of config will hang till I do release of sevntu. I will not need you to help, rease is done, PR is merged.

How can I continue, if we keep playing ping pong?

easy, just perform all points and I will merge you quickly. All follow such rules, you will not be exception.

The results from the target directory and the configuration used are here: checkstyle-tester-results-enum-trailing-comma-20161208.zip

please read README for checkstyle-tester, we need report to be placed to web as web site (to github.io if you do not have anything else)

[romani]

@kariem , ping .

[romani]

@kariem , ping.
Please let me know if you want to drop this PR , smb else might finish your work.

[romani]

@kariem , ping.

[rnveach]

@kariem Are you planning to finish this? I see you made a new push.
Branch has conflicts. I recommend squashing all commits and rebasing on top of master with no merge commit.

[kariem]

@rnveach thank you for the recommendation. Just saw that there are a few new checks that haven't been considered by my previous code.

[romani]

CIs failures should be resolved. All should be green.
Please fix commit message to follow format , see details at http://checkstyle.sourceforge.net/contributing.html#Submitting_your_contribution

[kariem]

@romani I have followed the recommendation to squash everything and rebase on top of master. Created a new one here: #709. It's OK to close this PR. It only has historical value (if any).

[rnveach]

Closing this as it is being moved to a new PR.