Add ignore-regex option to comments rule#720
Add ignore-regex option to comments rule#720peschmae wants to merge 1 commit intoadrienverge:masterfrom
Conversation
cb94c17 to
fd1f8ca
Compare
adrienverge
left a comment
There was a problem hiding this comment.
Hello @peschmae, thanks for contributing!
This looks good, but I have a few requests.
-
First, I disagree with myself of 2021 when I proposed
ignore-regexas the option name. Indeed, other rules don't use "regex" in their option names when their content should be a regex. Similarly, rules don't tell "option-…-string" when their value should be a string.Instead, I propose
ignore-comments. This will allow addingignore-linesin the future, if one day we want to silent errors based on the preceding YAML line contents, in case of inline comment:part of the line that should match the regex # comment -
Can you reword
Fixes #545toPartially fixes #545? Because the original request is about silenting both thecommentsandline-lengthrules, so in my opinion the issue #545 should remain open. -
Finally, write a good commit message (basically: the commit title should be
comments: …and the message could be what you wrote in the first PR message).
| # If the comment matched the ignore-regex, we don't perform any checks | ||
| if (len(conf['ignore-regex']) > 0 and | ||
| any(re.search(r, str(comment)) | ||
| for r in conf['ignore-regex'])): | ||
| return |
There was a problem hiding this comment.
- You can remove the comment: the code is self-explainatory.
if len(conf['ignore-regex']) > 0is not needed, becauseany([])is false.- Can you please add an empty line afterwards, like 7 lines below ↓, for logical separation?
| # If the comment matched the ignore-regex, we don't perform any checks | |
| if (len(conf['ignore-regex']) > 0 and | |
| any(re.search(r, str(comment)) | |
| for r in conf['ignore-regex'])): | |
| return | |
| if any(re.search(r, str(comment)) for r in conf['ignore-regex']): | |
| * ``ignore-regex`` is used to exclude specific comments from this check. | ||
| Only the comment itself will be matched to the regex, not the whole line on | ||
| inline comments. |
There was a problem hiding this comment.
- There are multiple syntax for regexes, here it's PCRE regexes. We mention it in
key-orderingandquoted-stringsdocumentations, so let's do it here too 👍 - I have a proposal that I find slightly clearer about inline comments:
| * ``ignore-regex`` is used to exclude specific comments from this check. | |
| Only the comment itself will be matched to the regex, not the whole line on | |
| inline comments. | |
| * ``ignore-comments`` is a list of PCRE regexes to exclude specific comments | |
| from this check. For inline comments, only the comment itself will be matched | |
| to the regex, not the whole line. |
What do you think?
| #. With ``comments: {ignore-regex: [^#cloud-config]}`` | ||
|
|
||
| the following code snippet would **PASS**: | ||
| :: | ||
|
|
||
| #cloud-config | ||
|
|
||
| #. With ``comments: {ignore-regex: [^#noqa]}`` | ||
|
|
||
| the following code snippet would **PASS**: | ||
| :: | ||
|
|
||
| enabled: true #noqa |
There was a problem hiding this comment.
I propose to:
- merge both examples to ease the reader understanding (both cases pass),
- give an example where the regex matches a part of the comment.
This would yield:
| #. With ``comments: {ignore-regex: [^#cloud-config]}`` | |
| the following code snippet would **PASS**: | |
| :: | |
| #cloud-config | |
| #. With ``comments: {ignore-regex: [^#noqa]}`` | |
| the following code snippet would **PASS**: | |
| :: | |
| enabled: true #noqa | |
| #. With ``comments: {ignore-regex: [^#cloud-config, ^#noqa]}`` | |
| the following code snippet would **PASS**: | |
| :: | |
| #cloud-config | |
| import lib #noqa: E402 |
(This doesn't make sense to fix a Cloud-init file with Python code, but at least it's short and readable.)
| '#but not this\n', conf, | ||
| problem1=(3, 2)) |
There was a problem hiding this comment.
To check close-but-not-exact matches:
| '#but not this\n', conf, | |
| problem1=(3, 2)) | |
| '#but not this\n' | |
| '# ignore this\n', conf, | |
| problem1=(3, 2), | |
| problem2=(4, 2)) |
|
This would be a very helpful feature! Is there any news on it? |
Fixes #545
This adds a
ignore-regexflag to thecommentsrule.If a comment matches the regex,
require-starting-spaceas well asmin-spaces-from-contentare skipped.The regex is only matched against the comment itself, and not against the full line.