Skip to content
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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idea: "allowed_noqa" Config Option #13

Open
erikvanderwerf opened this issue Jun 23, 2022 · 8 comments
Open

Idea: "allowed_noqa" Config Option #13

erikvanderwerf opened this issue Jun 23, 2022 · 8 comments

Comments

@erikvanderwerf
Copy link

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
Copy link
Owner

plinss commented Jun 24, 2022

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
Copy link

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
Copy link

ivanlonel commented Aug 11, 2022

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
Copy link
Owner

plinss commented Aug 11, 2022

@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
Copy link

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
Copy link
Author

erikvanderwerf commented Nov 16, 2022

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
Copy link

Avasam commented Oct 28, 2023

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 #22 ?

@erikvanderwerf
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants