plinss / flake8-noqa

flake8 plugin to validate #noqa comments - Mirror of https://gitlab.linss.com/open-source/flake8/flake8-noqa
GNU Lesser General Public License v3.0
40 stars 6 forks source link

Idea: "allowed_noqa" Config Option #13

Open erikvanderwerf opened 2 years ago

erikvanderwerf commented 2 years ago

Hello!

I think a great addition to this library would be an allowed_noqa config option (name fungible 😆), which allows a project lead to specify a global set of noqa comments that are allowed. This gives a dedicated place for notes and comments to developers regarding how those specific whitelisted noqa items can be used.

config.ini

allowed_noqa =
    # Relative Imports. Never for top-of-file imports in normal code. Only where relative imports
    # makes natural sense.
    ABS101
    # Missing a :raises: section. Addresses limitation by darglint. Never to suppress legitimate
    # lack of documentation. Only for `raise`-ing a variable.
    DAR401
    # Function name not lowercase. Never to purposefully violate naming standards. Only
    # for when overriding existing method names from library code.
    N802

All other noqa comments would be flagged by this plugin. I think this is useful because code review does not capture everything, and having a central location that explains the purpose of each noqa could be helpful.

If I'm missing a Flake feature that already does something like that- my bad and please let me know!

plinss commented 2 years ago

Interesting idea, and wouldn't be hard to add. I'm curious though, why you wouldn't simply add the allowed codes to flake8's ignore list? That way you'd never have to add # noqas to begin with. You could also then add disable-noqa to make any stray # noqas not work. (It might also be worthwhile to add a switch to flake8-noqa to disallow # noqas entirely...)

ivanlonel commented 2 years ago

why you wouldn't simply add the allowed codes to flake8's ignore list?

I've come accross one case where an allowed_noqa option of sorts would be helpful.

I'm using flake8-broken-line to flag the use of \ in strings, and I want flake8 to report it on most cases.

There are however a few exceptions where I'd like to allow \ in docstrings to keep stuff in a single line without exceeding line length limit or having to break the docstring. But to do that I need to specify # noqa: N400 after the closing quotes of the docstring, which are not on the same line as the offending \, so I'm getting a NQA102 instead.

ivanlonel commented 2 years ago

Of course in this specific case it would be even better if flake8-noqa could just consider the whole multiline string literal as a single line for validation purposes, since we can't add # noqa comments inside those anyway. 😁

plinss commented 2 years ago

@ivanlonel what you're describing sounds like a different issue, there have ben recent improvements in recognizing violations in multi-line tokens (like docstrings). Are you using the latest version?

ivanlonel commented 2 years ago

Thanks for the quick reply, @plinss.

Yes, I'm using flake8-noqa==1.2.8 with flake8==5.0.4 on a fresh Python 3.10.5 venv.

Here's a simple example where this happens:

image image

erikvanderwerf commented 1 year ago

Interesting idea, and wouldn't be hard to add. I'm curious though, why you wouldn't simply add the allowed codes to flake8's ignore list? That way you'd never have to add # noqas to begin with. You could also then add disable-noqa to make any stray # noqas not work. (It might also be worthwhile to add a switch to flake8-noqa to disallow # noqas entirely...)

Adding the #noqa code to the project's ignore list would not effectively capture the nuance here, because it would ignore legitimate violations that could be fixed. By creating what is effectively a whitelist of allowed #noqa codes, the project can declare which codes are even allowed to be used.

Granted, this could also be done with more thorough code reviews, but I like the accessibility of being able to document in the project config what each relevant noqa is meant for without having to memorize or research.

Here's an example of where broadly ignoring N802 is too much.

from overrides import overrides
from some.library import Egg

class Chicken(Egg):
    @overrides
    def HatchTheEgg(self):  # <-- noqa would be appropriate here since this comes from a library method.
        return ...

    def SaySomething(self):  # <-- Real N802 violation in our code that should be fixed.
        return "Bacawk!"
Avasam commented 10 months ago

Interesting idea, and wouldn't be hard to add. I'm curious though, why you wouldn't simply add the allowed codes to flake8's ignore list? That way you'd never have to add # noqas to begin with. You could also then add disable-noqa to make any stray # noqas not work. (It might also be worthwhile to add a switch to flake8-noqa to disallow # noqas entirely...)

If the code comes from a different tool than Flake8, then adding the code to the ignore list doesn't work. Although maybe that's more https://github.com/plinss/flake8-noqa/issues/22 ?

erikvanderwerf commented 10 months ago

@Avasam I think your comment aligns closer with the other issue than this one. My intention with this ask was to specify a high-level set of approved noqa lines that are even allowed to be used in the project, and would raise a lint flag on the mere existence of anything not specifically approved. This would not modify the behavior of any other checks, but prevents someone in a large team/codebase from ignoring any new kinds of checks without understanding and documenting the modification.