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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File-scoped flake8: noqa directive with comment is not recognized #10239

Closed
tylerlaprade opened this issue Mar 5, 2024 · 4 comments
Closed
Labels
suppression Related to supression of violations e.g. noqa

Comments

@tylerlaprade
Copy link
Contributor

# flake8: noqa -- comment explaining why we're disabling flake8 for this file

As per Ruff's documentation, this is treated the same as # ruff: noqa, which doesn't allow comments. However, flake8 does allow comments - and I happen to believe comments should be required any time some/all rules are disabled to explain why. This particular comment is in fact being used to disable flake8 for this file. The author would have liked to also disable Ruff for the whole file, but didn't realize that the comment could do both due to the comment, and ended up disabling the particular line with a Ruff violation.

This came to my attention when I upgraded to Ruff v0.3.0 (I'm very excited to try the new formatting, by the way!). I ran ruff check and got a warning every time:

warning: Invalid `# ruff: noqa` directive at abacus/migrations/0324_manual_recompute_all_patient_journeys.py:2: expected `:` followed by a comma-separated list of codes (e.g., `# noqa: F401, F841`).

Ideally, comments would be allowed on ruff: noqa. But even if not, Ruff should not be warning about a valid flake8 directive.

@charliermarsh
Copy link
Member

I think a format we could allow (which would be consistent with # noqa) would be like: # flake8: noqa # some comment.

@charliermarsh charliermarsh added the suppression Related to supression of violations e.g. noqa label Mar 5, 2024
@tylerlaprade
Copy link
Contributor Author

I got in the habit of using -- from ESLint, but any consistent format would be good, and I think it'd be good if Ruff chose a single format, whatever that is, to help the community standardize (and to encourage commenting on why rules are suppressed).

That being said, anything that Flake8 currently allows should at the very least not trigger a Ruff warning, since a large legacy codebase might be discouraged from migrating if they see hundreds of warnings on previously valid code.

@zanieb
Copy link
Member

zanieb commented Mar 11, 2024

Related discussion at #8876 (comment)

@zanieb
Copy link
Member

zanieb commented Mar 11, 2024

Going to track this in #10343 instead since this is not a flake8-specific problem.

Thank you for raising! If you have thoughts on the design please respond over there.

@zanieb zanieb closed this as not planned Won't fix, can't repro, duplicate, stale Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suppression Related to supression of violations e.g. noqa
Projects
None yet
Development

No branches or pull requests

3 participants