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

Update google_checks.xml to have the SuppressionCommentFilter and SuppressWarningsHolder modules in the config by default (and by extension, SuppressWarningsFilter) #11655

Closed
ddcprg opened this issue May 18, 2022 · 11 comments · Fixed by #11656

Comments

@ddcprg
Copy link
Contributor

ddcprg commented May 18, 2022

I have downloaded the latest cli from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

How it works Now:

/var/tmp $ javac MyClass.java
#[[MAKE SURE THERE IS SUCCESSFUL COMPILATION]]

/var/tmp $ cat google_checks.xml
...
<module name="Checker">
  <!-- IMPORTANT: Allows use of SuppressWarnings annotation -->
  <module name="SuppressWarningsFilter"/>
  ...
  <module name="TreeWalker">
    <module name="SuppressWarningsHolder" />
    <module name="SuppressionCommentFilter">
      <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ([\w\|]+)" />
      <property name="onCommentFormat" value="CHECKSTYLE.ON\: ([\w\|]+)" />
      <property name="checkFormat" value="$1" />
    </module>
...
</module>


/var/tmp $ cat MyClass.java
class MyClass {
  public void aMethod() {
  }
}


/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-10.2-all.jar -c google_checks.xml MyClass.java
Starting audit...
[WARN] /var/tmp/MyClass.java:2:15: Method name 'aMethod' must match pattern '^[a-z][a-z0-9][a-zA-Z0-9_]*$'. [MethodName]
Audit done.

After using @SuppressWarnings

/var/tmp $ cat MyClass.java
class MyClass {
  @SuppressWarnings("checkstyle:methodname")
  public void aMethod() {
  }
}


/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-10.2-all.jar -c google_checks.xml MyClass.java
Starting audit...
Audit done.

Using special comments to switch checkstyle off and on:

/var/tmp $ cat MyClass.java
class MyClass {
  // CHECKSTYLE.OFF: MethodName
  public void aMethod() {
  }
  // CHECKSTYLE.ON: MethodName
}


/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-10.2-all.jar -c google_checks.xml MyClass.java
Starting audit...
Audit done.

Is your feature request related to a problem? Please describe.
No, this is just additional config in google_checks.xml.

Describe the solution you'd like
Add the ability to suppress style checks via annotations and special comments around the code. This will offer users alternatives to suppress checks other than using XML.

Additional context
Some of these changes have been already adopted by Google https://github.com/GoogleCloudPlatform/java-repo-tools/blob/main/third_party/checkstyle/google_checks.xml

References

@nrmancuso
Copy link
Contributor

nrmancuso commented May 19, 2022

@ddcprg please remove all modules from your config that are not related to this issue to make your example easier to read and understand.

@ddcprg
Copy link
Contributor Author

ddcprg commented May 19, 2022

@nick-mancuso I've updated the config to show the relevant bits only

@nrmancuso
Copy link
Contributor

nrmancuso commented May 19, 2022

@ddcprg to be clear, you would like google_checks.xml to have the SuppressionCommentFilter and SuppressWarningsHolder modules in the config by default (and by extension, SuppressWarningsFilter), correct?

@ddcprg
Copy link
Contributor Author

ddcprg commented May 19, 2022

that's correct @nick-mancuso I've raised PR even though the ticket hasn't been approved #11656

@nrmancuso
Copy link
Contributor

nrmancuso commented May 19, 2022

This request makes sense to me, since we currently do not support anything that makes inheritance/ override in configs possible, such as #2873. @romani what do you think?

@nrmancuso
Copy link
Contributor

nrmancuso commented May 19, 2022

@ddcprg please update issue title to be more along the lines of #11655 (comment), make sure to mention modules you want and google_checks.xml, please.

@ddcprg ddcprg changed the title Allow suppressing warning via annotations and special comments Update google_checks.xml to have the SuppressionCommentFilter and SuppressWarningsHolder modules in the config by default (and by extension, SuppressWarningsFilter) May 19, 2022
@ddcprg
Copy link
Contributor Author

ddcprg commented May 19, 2022

@nick-mancuso thanks, copy&pasted now - almost

ddcprg added a commit to ddcprg/checkstyle that referenced this issue May 19, 2022
@romani
Copy link
Member

romani commented May 20, 2022

Users should be able to suppress violations in way they want.
We might need need to add something like

<module name="SuppressWithNearbyCommentFilter">
also to let users use comments also

@nrmancuso
Copy link
Contributor

nrmancuso commented May 20, 2022

Approved, in PR for this issue, we should do:

  1. Add SuppressWithNearbyCommentFilter, SuppressionCommentFilter, and SuppressWarningsHolder modules to google_checks.xml
  2. Add SuppressWarningsFilter module to google_checks.xml
  3. Update documentation at https://checkstyle.sourceforge.io/google_style.html#Google_Suppressions

@ddcprg
Copy link
Contributor Author

ddcprg commented May 25, 2022

thanks @nick-mancuso @romani I'll include the additional module and fix the PR

ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 6, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 6, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 6, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 6, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 6, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 6, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 8, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 8, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 9, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 29, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jun 29, 2022
ddcprg added a commit to ddcprg/checkstyle that referenced this issue Jul 5, 2022
@nrmancuso nrmancuso added this to the 10.4 milestone Jul 8, 2022
@nrmancuso
Copy link
Contributor

nrmancuso commented Jul 8, 2022

Fix was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants