Specific error codes specified with # NOQA are not detected when no colon is used

[asottile]

In GitLab by @jeverling on Oct 26, 2018, 05:01

Omitting flake8 --bug-report and "how I installed flake8" because the bug exists in the master branch of the flake8 repo.

The regex that is used to check if there are error codes provided with a # NOQA instruction doesn't work without a colon, but it should, according to the comment in defaults.py#L33:

# We do not care about the ``: `` that follows ``noqa``

Actually the regex does not capture the error code when no colon is used, resulting in only # NOQA which suppresses all errors.

In [1]: import re

In [2]: re.compile(r"# noqa(?:: (?P<codes>([A-Z][0-9]+(?:[,\s]+)?)+))?", re.IGNORECASE).match('# NOQA: F401').groupdict()['codes'] == 'F401'
Out[2]: True

In [3]: re.compile(r"# noqa(?:: (?P<codes>([A-Z][0-9]+(?:[,\s]+)?)+))?", re.IGNORECASE).match('# NOQA F401').groupdict()['codes'] == 'F401'
Out[3]: False

[asottile]

In GitLab by @jeverling on Oct 26, 2018, 07:02

mentioned in merge request !260

[asottile]

In GitLab by @asottile on Oct 26, 2018, 08:29

heh, I had the same misunderstanding about that code comment (it's referring to regex capture, not the functionality of the regex itself).

[asottile]

In GitLab by @jeverling on Oct 26, 2018, 09:01

Oooh, I searched but somehow missed that issue. Thanks for the explanation,
I wasn't aware of non-capturing groups. I'll update the documentation and
revert the change to the test tomorrow.
Thanks and have a nice weekend!

[asottile]

In GitLab by @asottile on Oct 26, 2018, 09:27

It might be worth doing this anyway.

I've seen these "mistake" forms, and we might want to just match those anyway?

• # noqa:E123
• # noqa E123

curious to hear what @sigmavirus24 thinks on this subject :)

[asottile]

In GitLab by @sigmavirus24 on Oct 27, 2018, 05:37

The : in the comment had a very specific reason behind it - namely there are a few projects that rely on # noqa (with varying strictness in how they match it) to turn something off. The idea then was to have something following it immediately to make it clear that this was not for use with those systems. I think some of them have regular expressions that look like # ?noqa\b or # ?noqa$ etc. So adding a : avoids either of those being matched by accident.

I'd be open to # noqa:E123 (optional space after the :) but not the second variant. It has problems for other tools.

Further, other tools have adopted the syntax already so making too many changes will have knock-on effects elsewhere and I don't hate other developers/maintainers enough to put that onus on them.

[asottile]

In GitLab by @jeverling on Oct 28, 2018, 12:18

mentioned in merge request !262

[asottile]

In GitLab by @jeverling on Oct 28, 2018, 12:24

Thanks @sigmavirus24 for the explanation, that's a good reason. Like @asottile I have seen the # noqa E123 form in the wild, that's why I started to use it and then was surprised it would lead to all errors being ignored. But OTOH the documentation clearly includes the colon. I made some small changes in a new MR (!262), to make it clearer that a) the colon should not be captured in the regex and b) the colon is significant.

[asottile]

In GitLab by @jeverling on Oct 28, 2018, 13:25

closed via commit cafe780

[asottile]

In GitLab by @asottile on Oct 28, 2018, 13:25

closed via merge request !262

[asottile]

In GitLab by @asottile on Oct 28, 2018, 13:25

mentioned in commit 732a466

[asottile]

In GitLab by @asottile on Oct 28, 2018, 13:27

Reopening in case someone wants to tackle the # noqa:E123 case

[asottile]

In GitLab by @asottile on Oct 28, 2018, 13:27

reopened

[asottile]

In GitLab by @sigmavirus24 on Oct 28, 2018, 17:19

What about creating a new issue for that? It'll be clearer what is desired
and can be marked as low difficulty

[asottile]

In GitLab by @asottile on Oct 28, 2018, 17:21

sounds good -- I'll do that 👍

[asottile]

In GitLab by @asottile on Oct 28, 2018, 17:21

closed