PyCQA / flake8

flake8 is a python tool that glues together pycodestyle, pyflakes, mccabe, and third-party plugins to check the style and quality of some python code.
https://flake8.pycqa.org
Other
3.39k stars 306 forks source link

file-level noqa should raise an error if specific errors are provided (to avoid confusion) #1793

Open bagerard opened 1 year ago

bagerard commented 1 year ago

how did you install flake8?

pip install flake8

unmodified output of flake8 --bug-report

{
  "platform": {
    "python_implementation": "CPython",
    "python_version": "3.10.7",
    "system": "Linux"
  },
  "plugins": [
    {
      "plugin": "mccabe",
      "version": "0.7.0"
    },
    {
      "plugin": "pycodestyle",
      "version": "2.10.0"
    },
    {
      "plugin": "pyflakes",
      "version": "3.0.1"
    }
  ],
  "version": "6.0.0"
}

describe the problem

what I expected to happen

I'm seeing developers using the file-level-noqa ignore wrongly from time to time, providing specific errors like # flake8: noqa: E712. What's confusing is that it gives the impression that it works as it doesn't throw that error anymore but the developers' understanding is incorrect (all errors are ignore). I understood that you don't want to support the file-level-specific-ignores but I think flake8 should do something to prevent this confusion.

See for instance https://til.codeinthehole.com/posts/filelevel-flake8-comments-ignore-all-errors/

My suggestion would be to detect that and throw an error? Somehow it is like providing a flake8 config with a wrong format

sample code

# flake8: noqa: E712

from os import *       # F401
assert True == True # E712

Let me know what you think, happy to work on this if you would welcome a PR

asottile commented 1 year ago

given how much people were mad at 6.0 pointing out a common mistake I'm hesitant. I don't need another 2 weeks of needing to lock down the issue tracker while people scream at me in my email

bagerard commented 1 year ago

I understand the hesitation then :grimacing:. Thanks for the quick feedback and for your efforts in Python's ecosystem :rocket:

asottile commented 1 year ago

the "simplest" fix for this would be to slap a $ on the file regex

sigmavirus24 commented 1 year ago

I almost feel like there needs to be a "mode" for linting people's use of flake8. Like flake8 --warn-on-misuse that detects stuff like this so people could use that to prevent nonsense like this from creeping in. It would be forever a "beta" feature that adds new things to it. Could be used for deprecation warnings of upcoming breaking releases too. Sadly it'll only draw more ire because we don't just accept every feature request to make the tool less useful to large teams.

jakkdl commented 1 year ago

@sigmavirus24 you can suggest it for https://github.com/PyCQA/flake8-bugbear

LeXofLeviafan commented 11 months ago

My suggestion would be to detect that and throw an error

I'd argue the opposite: actually support the thing that users are trying to do.

This functionality is implemented already (as a CLI/config file option); it shouldn't cost that much effort to detect if the # flake8: noqa comment also specifies a list of errors (in exactly the same way as per-line # noqa comments do), and extend per-file-ignores list instead of the exclude one.

the "simplest" fix for this would be to slap a $ on the file regex

Well that sounds an awful lot like "quietly ignoring the directive" which is hardly a good behaviour for any kind of file processor… (Not to mention, the current behaviour of # noqa allowing text after the error list is actually useful – it allows to provide justification for using the directive within the same comment.)

Kolumbs commented 6 months ago

ChatGPT even suggests to use # flake8: noqa: E501 for file level error code silencing :)

But anyway what is the issue to actually allow this feature so that one can easily manage error suppressing per file in the file where one writes the code. This is arguably good feature?

At the moment I use following in flake config file: per-file-ignores = my_file_folder/my_file.py: E501

But not optimal imho.

And yes I agree to @LeXofLeviafan instead of throwing error let's just allow the feature

asottile commented 6 months ago

chatgpt is hot garbage

sigmavirus24 commented 6 months ago

As a reminder to people saying "we should just do it" you are given software AS IS per the license. Unless you're maintaining the software, "just do it" isn't a justification. Please read https://blog.ian.stapletoncordas.co/2023/11/no-should-be-your-default before spewing that mentality around other projects