Skip to content

Suppress Checkstyle within text-blocks #323

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.

Sign up for GitHub

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

Closed
hashhar wants to merge 1 commit into from
Closed

Suppress Checkstyle within text-blocks #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Airbase 129

* Checkstyle updates:
- Require package name to match the source directory
- Suppress Checkstyle within text-blocks
* Dependency updates:
- joda-time 2.10.14 (from 2.10.13)
- slf4j 1.7.36 (from 1.7.32)
Expand Down
7 changes: 7 additions & 0 deletions airbase-policy/src/main/resources/checkstyle/airbase-checks.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
"http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd">
<module name="Checker">
<!-- TODO: Remove this supression once RegexpMultiline has been replaced with an AST aware check -->
Copy link
Copy Markdown
Contributor

@findepi findepi Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What can be done here?

<!-- This suprpresses Checkstyle between onccurences of """", i.e. text blocks -->
Copy link
Copy Markdown
Contributor

@findepi findepi Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: suprpresses -> suppresses

<module name="SuppressWithPlainTextCommentFilter">
<property name="offCommentFormat" value='=\s+"""'/>
Copy link
Copy Markdown
Contributor

@findepi findepi Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why =?

<property name="onCommentFormat" value='^\s+.*""";'/>
Copy link
Copy Markdown
Contributor

@findinpath findinpath Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take into consideration the following use case:

  • the text block is a parameter among other parameters to a method call (can be followed by , or )

Copy link
Copy Markdown
Contributor Author

@hashhar hashhar Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that makes it slightly challenging. Since we need some way to differentiate opening and closing text block markers. One way to do this is to enforce all text-block content appears on a newline by changing start marker to \s+"""$.

Changing closing marker to ^\s+.*""".+ would mean it can still match opening marker if we don't enforce that text blocks start on a new line.

Any suggestions?
In meantime I'm looking at replacing RegexpMultiline.

Copy link
Copy Markdown
Contributor

@findinpath findinpath Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess your solution is based on the proposal

checkstyle/checkstyle#11653 (comment)

What if we use something like:

<property name="onCommentFormat" value='^\s+.*"""(;|,|\))'/>

?

Copy link
Copy Markdown
Contributor Author

@hashhar hashhar Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That covers most of the cases we know of. Thanks. Hopefully this is a short term solution so we don't need to make it pretty.

Copy link
Copy Markdown
Contributor

@findinpath findinpath Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few minor variations when dealing with ), it can be followed optionally by other ) and eventually by , or ;. Good thing is that AFAIK they are all on the same line.

<property name="onCommentFormat" value='^\s+.*"""(,|;|(\)+(,|;)))'/>

NOTE that I tested this so far only on regex101.com and not with the changes on your branch.

Copy link
Copy Markdown
Contributor

@findepi findepi Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • the text block is a parameter among other parameters to a method call (can be followed by , or )

i think this is important.
It's confusing to allow

String x = """
   )
 """;

while not allow

assertQuery(
   """
    )
    """);

the text blocks are awesome for SQL queries and SQL queries are awesome to inline in tests, so this would be primary use-case

</module>

<module name="FileTabCharacter" />
<module name="NewlineAtEndOfFile">
<property name="lineSeparator" value="lf" />
Expand Down