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 filter: SuppressWithNearbyTextFilter to suppress checks with inline comments in any file type #11939

Closed
stoyanK7 opened this issue Jul 20, 2022 · 13 comments · Fixed by #12151

Comments

@stoyanK7
Copy link
Contributor

stoyanK7 commented Jul 20, 2022

From #11884

Description

The filter should work exactly like SuppressWithNearbyCommentFilter. The only difference is that it should be able to filter different file types(i.e. have Checker as parent module instead of TreeWalker) - java, xml, js, properties, sql, etc. It shoud have the following properties:

Name Description Default value
commentFormat Specify comment pattern to trigger filter to begin suppression. "SUPPRESS CHECKSTYLE (\w+)"
checkFormat Specify check pattern to suppress. ".*"
messageFormat Define message pattern to suppress. null
idFormat Specify check ID pattern to suppress. null
influenceFormat Specify negative/zero/positive value that defines the number of lines preceding/at/following the suppression comment. "0"

How it should work

This is just an example config with 3 files to show what is expected. Configuration of properties should be the same as the above mentioned filter. Their behaviour is well-documented there.

Config

<?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="NewFilter"/>
	
    <module name="LineLength">
         <property name="max" value="2"/>
    </module>
</module>

XML

<something/> <!-- SUPPRESS CHECKSTYLE LineLengthCheck --> // ok
<somethingelse/> // violation

Expected output

$ java -jar checkstyle-all.jar -c config.xml file.xml
[ERROR] file.xml:3:0: Line is longer than 2 characters (found 16). [LineLength]

SQL

SELECT COUNT(1) FROM filters; -- SUPPRESS CHECKSTYLE LineLengthCheck // ok
SELECT COUNT(1) FROM checks; // violation

Expected output

$ java -jar checkstyle-all.jar -c config.xml file.sql
[ERROR] file.sql:2:0: Line is longer than 2 characters (found 28). [LineLength]

Java

int a = 5; // SUPPRESS CHECKSTYLE LineLengthCheck // ok
int b = 3; // violation

Expected output

$ java -jar checkstyle-all.jar -c config.xml file.java
[ERROR] file.java:2:0: Line is longer than 2 characters (found 10). [LineLength]

Config with influenceFormat

<?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="NewFilter">
        <property name="influenceFormat" value="2"/>
    </module>

	
    <module name="LineLength">
        <property name="max" value="2"/>
    </module>
</module>

XML

<something/> <!-- SUPPRESS CHECKSTYLE LineLengthCheck --> // ok
<somethingelse/> // ok
<somethingelse1/> //ok
<somethingelse2/> // violation

An aggreement on name needs to be made. For the demo purpose I just called it NewFilter.

@stoyanK7
Copy link
Contributor Author

I guess I will roughly start working on this.

@romani
Copy link
Member

romani commented Jul 25, 2022

<?xml version="1.0" encoding="UTF-8"?>
<something/> <!-- SUPPRESS CHECKSTYLE LineLengthCheck --> // ok

please remove first line or put violation on it.


please write example with influenceFormat


can you suggest a name for new Filter ?

@stoyanK7
Copy link
Contributor Author

stoyanK7 commented Jul 25, 2022

Here are e few suggestions. Let me know what you think about them.

  • SuppressWithAreaCommentFilter
  • SuppressWithClosebyCommentFilter
  • SuppressWithProximateCommentFilter

First two sound good, last one not so much.

@romani
Copy link
Member

romani commented Jul 27, 2022

I think your new filter will be Checker version of https://checkstyle.org/config_filters.html#SuppressWithNearbyCommentFilter

what about SuppressWithNearbyPlainTextCommentFilter?
as you mostlikely work fine in any other languages with special comments # comment , -- comment, !-- comment, <!-- comment -->

@stoyanK7
Copy link
Contributor Author

SuppressWithNearbyPlainTextCommentFilter sounds good. No complaints there.

@nrmancuso
Copy link
Member

Just a thought: it would be great to allow influenceFormat to be baked into comment. Taking a page from our "BDD":

<?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="NewFilter"/>
    <module name="LineLength">
        <property name="max" value="2"/>
    </module>
</module>
<something/> <!-- SUPPRESS CHECKSTYLE LineLengthCheck (2)--> // ok
<somethingelse/> // ok
<somethingelse1/> //ok
<somethingelse2/> // violation

<something/> <!-- SUPPRESS CHECKSTYLE LineLengthCheck (1)--> // ok
<somethingelse/> // ok
<somethingelse1/> // violation
<somethingelse2/> // violation

<something/> <!-- SUPPRESS CHECKSTYLE LineLengthCheck (3)--> // ok
<somethingelse/> // ok
<somethingelse1/> // ok
<somethingelse2/> // ok

@stoyanK7
Copy link
Contributor Author

stoyanK7 commented Jul 27, 2022

SuppressWithNearbyCommentFilter allows this by doing. Since code will mostly be copied from there the following should be possible:

<property name="commentFormat"
      value="CHECKSTYLE IGNORE (\w+) FOR NEXT (\d+) LINES"/>
<property name="checkFormat" value="$1"/>
<property name="influenceFormat" value="$2"/>
static final int lowerCaseConstant; // CHECKSTYLE IGNORE ConstantNameCheck FOR NEXT 3 LINES
static final int lowerCaseConstant1;
static final int lowerCaseConstant2;
static final int lowerCaseConstant3;
static final int lowerCaseConstant4; // will warn here

Taken from here(scroll a bit down to see example).

Or do you expect your proposal to be possible as well?

@nrmancuso
Copy link
Member

nrmancuso commented Jul 27, 2022

@stoyanK7 in your example above(in issue description) the check ID property is not in the config, which implies that this is now “baked in” to the default config for this filter. I was thinking that if we are making check ID capture group as the first capture group, we should do the same for influenceFormat as the second group. Or, was this just an omission?

@stoyanK7
Copy link
Contributor Author

@nick-mancuso I'm sorry, I don't get what you meant here. Could you explain in other words?

@nrmancuso
Copy link
Member

@stoyanK7 all will become clear in PR, let’s not worry about this now.

@rnveach
Copy link
Member

rnveach commented Mar 5, 2023

@stoyanK7 I don't think your config example makes sense or is valid. LineLength is part of TreeWalker in your example, but TreeWalker modules are only executed on Java files while your example input is XML. Also, LineLength is not a TreeWalker module.

CheckstyleException: TreeWalker is not allowed as a parent of LineLength Please review 'Parent Module' section for this Check in web documentation if Check is standard.

@stoyanK7
Copy link
Contributor Author

stoyanK7 commented Mar 5, 2023

@rnveach This must have been a mistake, thanks for pointing it out. Fixing it right now.

@nrmancuso
Copy link
Member

Closed via #12151

Many thanks to @stoyanK7 for this new filter, this was a huge effort

@nrmancuso nrmancuso added this to the 10.9.4 milestone Apr 26, 2023
@romani romani changed the title New filter: Suppress checks with inline comments on different file types New filter: SuppressWithNearbyTextFilter to suppress checks with inline comments in any file type Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants