Suppress Checkstyle within text-blocks#323
Suppress Checkstyle within text-blocks#323hashhar wants to merge 1 commit intoairlift:masterfrom hashhar:hashhar/textblocks
Conversation
Some checks like RegexpMultiline are not AST aware and hence cannot differentiate between content inside a text-block or outside of it. This is a short term workaround which uses the """ text as a marker to let Checkstyle suppress any violations between them. The longer term fix is to migrate away from non-AST aware checks like RegexpMultiline and RegexpSingleline to something else.
|
I've been looking at some alternative checks to migrate the Regexp checks to. So far it seems EmptyLineSeparator can be configured to handle some cases but it doesn't cover everything. If I don't find an existing check I'll try writing a new one that applies all vertical whitespace related rules we have today. |
| <!-- This suprpresses Checkstyle between onccurences of """", i.e. text blocks --> | ||
| <module name="SuppressWithPlainTextCommentFilter"> | ||
| <property name="offCommentFormat" value='=\s+"""'/> | ||
| <property name="onCommentFormat" value='^\s+.*""";'/> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess your solution is based on the proposal
checkstyle/checkstyle#11653 (comment)
What if we use something like:
<property name="onCommentFormat" value='^\s+.*"""(;|,|\))'/>
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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
| "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 --> | ||
| <!-- This suprpresses Checkstyle between onccurences of """", i.e. text blocks --> |
There was a problem hiding this comment.
nit: suprpresses -> suppresses
| <!-- TODO: Remove this supression once RegexpMultiline has been replaced with an AST aware check --> | ||
| <!-- This suprpresses Checkstyle between onccurences of """", i.e. text blocks --> | ||
| <module name="SuppressWithPlainTextCommentFilter"> | ||
| <property name="offCommentFormat" value='=\s+"""'/> |
| "-//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 --> |
|
@hashhar that would be great to write a new checker that can replace the regex checks. I've wanted to do that for a while. It's probably not hard to add a new Checkstyle module to Airbase, or possibly add it into the existing |
|
I'd like to add it but unfortunately I'm not actively working on this at the moment. I'll probably get to it in the next week and it's free for someone else to take a shot at this until then. |
4 participants
Some checks like RegexpMultiline are not AST aware and hence cannot
differentiate between content inside a text-block or outside of it. This
is a short term workaround which uses the """ text as a marker to let
Checkstyle suppress any violations between them.
The longer term fix is to migrate away from non-AST aware checks like
RegexpMultiline and RegexpSingleline to something else.
cc: @kokosing @martint
Fixes trinodb/trino#14623