Skip to content
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

new check: UnnecessarySemicolonInEnumeration #6773

Closed
strkkk opened this issue May 23, 2019 · 10 comments

Comments

Projects
None yet
4 participants
@strkkk
Copy link
Contributor

commented May 23, 2019

From #6752 (comment)
When enum has nothing but constants declaration, trailing semicolon is unnecessary.
It should be detected and reported as violation

$ cat TestClass.java
enum Nothing {
  A,B
}
enum Comma {
  A,B,
}
enum Semicolon {
  A,B; // violation
}
enum EvenThis {
  A,B,; // violation
}

enum Normal {
A,B; // OK
Normal(){}
}

$ cat conf.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
       <module name="UnnecessarySemicolonInEnumeration" />
    </module>
</module>

$ java -jar checkstyle-8.20-all.jar -c conf.xml Test.java
Starting audit...
here is violations at lines commented with // violation
@romani

This comment has been minimized.

Copy link
Member

commented May 25, 2019

please add to test case:

enum EvenThis {
  A,
  B,
  ; // required ";", no violation
  Normal(){}
}

If there is something in Enum other than elements, ";" is requirement, no violations in any form of code(minimization of diff in multiline mode is not applicable here).

I am ok with design.

@rnveach and/or @pbludov , please review and approve.

@rnveach

This comment has been minimized.

Copy link
Member

commented May 25, 2019

I am ok.

@rnveach rnveach added the approved label May 25, 2019

@pbludov

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

I think we also need a check like ArrayTrailingComma:
Every constant in simple enum should ends with a comma, for the same reason.
This check will forbid semicolon at the end, and a separate EnumConstantTrailingComma check will require a comma at the end.

UPD: we (almost) have it:
sevntu-checkstyle/sevntu.checkstyle#464

strkkk added a commit to strkkk/checkstyle that referenced this issue May 26, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 26, 2019

@strkkk

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

@romani

please add to test case:

I added to test cases many things (like parentheses/comma/block and its combinations) including semicolon on next line.
I included your example into docs.

Test cases can be found in #6782

@romani

This comment has been minimized.

Copy link
Member

commented May 27, 2019

I think we also need a check like

this is separate discussion, if you see that something new should be done, please open new issue (discussion thread).
Please confirm that you are ok with proposed design of Check in this issue.

strkkk added a commit to strkkk/checkstyle that referenced this issue May 27, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 28, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 28, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 29, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue May 30, 2019

romani added a commit that referenced this issue Jun 1, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Fix was merged

@rnveach rnveach closed this Jun 1, 2019

@rnveach rnveach added this to the 8.22 milestone Jun 1, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

@strkkk ,

We got CI failure in master after merge - https://circleci.com/gh/checkstyle/checkstyle/22189 :

Actual:
JavadocPropertiesGenerator.java.html:<td class='covered'><pre><span  class='survived'>            if (result) {</span></pre></td></tr>
Ignore:

Diff:
--- target/ignored.txt	2019-06-01 12:55:07.402379061 +0000
+++ target/actual.txt	2019-06-01 12:55:07.402379061 +0000
@@ -0,0 +1 @@
+JavadocPropertiesGenerator.java.html:<td class='covered'><pre><span  class='survived'>            if (result) {</span></pre></td></tr>
Difference between 'Actual' and 'Ignore' lists is detected, lists should be equal.
build will be failed.
Exited with code 1

I am not sure how this leak happened, but we need to fix it, as all further PRs will be failing in the same way.

@strkkk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

@romani this is really strange, I didn't touch this class in my PR.
I can check it later today.

@strkkk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

I played with this condition and tests always result in failure for mutations I made (remove condition, negate it)
Also, other PRs like #6797 pass pitest ok.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

CircleCI is passing in master now so I would assume this is a random error and not related to this issue.
It could be close to the edge of timing out, which would falsely flag it as being killed. Or it could be something random that causes to pass and fail on different machines.

thomassenger pushed a commit to thomassenger/checkstyle that referenced this issue Jun 6, 2019

@RohanNagar RohanNagar referenced this issue Jun 24, 2019

Open

Cleanup Tasks #232

3 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.