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.

Sign up for GitHub

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

Jump to bottom

Warn when # noqa: <code> contains an invalid code #3339

Closed
martinlehoux opened this issue Mar 4, 2023 · 9 comments · Fixed by #5571
Closed

Warn when # noqa: <code> contains an invalid code #3339

martinlehoux opened this issue Mar 4, 2023 · 9 comments · Fixed by #5571
Assignees
@charliermarsh
Labels
suppression Related to supression of violations e.g. noqa

Comments

@martinlehoux
Copy link
Contributor

With the following snippet, I can ignore the issue in 3 ways, but the 4th fails to ignore, and it's not consistant:

  • Add # noqa: N815 on the line ✔️
  • Add # noqa: mixed-case-variable-in-class-scope on the line ✔️
  • Add # ruff: noqa: N815 on the first line of the file ✔️
  • Add # ruff: noqa: mixed-case-variable-in-class-scope on the first line of the file 🛑
from typing import TypedDict


class Request(TypedDict):
    queryString: str -> Variable `queryString` in class scope should not be mixedCaseRuff(N815)
@charliermarsh
Copy link
Member

charliermarsh commented Mar 4, 2023 •
edited
Loading

The second way is actually kind of a bug but it's consistent with Flake8 IIUC -- # noqa: mixed-case-variable-in-class-scope ignores all errors on that line.

@martinlehoux
Copy link
Contributor Author

😮 Damn that could lead me to unexpected ignores, I thought it was normal behavior. I have # noqa: hardcoded-password-string or # noqa: ambiguous-unicode-character-string all over the place

@charliermarsh
Copy link
Member

I thought we warned about that, I'll have to double-check. (I see stuff like # noqa: F401 often, which Flake8 treats as "ignore all rules".)

We don't currently support using the human-readable names for ignores, we need to finalize the names first.

@charliermarsh
Copy link
Member

Wait sorry sorry.

@charliermarsh
Copy link
Member

I misspoke. Let me clarify.

@charliermarsh
Copy link
Member

charliermarsh commented Mar 4, 2023 •
edited
Loading

# noqa: F401 only ignores F401 on a given line.

# flake8: noqa: F401 on its own line ignores all errors in a file (which is a common mistake).

@charliermarsh
Copy link
Member

# noqa: mixed-case-variable-in-class-scope ignores all errors on a given line, erroneously (in both Ruff and Flake8).

@charliermarsh
Copy link
Member

If you do # ruff: noqa: mixed-case-variable-in-class-scope, we do properly warn that mixed-case-variable-in-class-scope is an unknown code.

@charliermarsh
Copy link
Member

We should probably warn that # noqa: mixed-case-variable-in-class-scope is an unknown code.

@charliermarsh charliermarsh changed the title Inconsistent ruff: noqa: <full-name> behavior on file first line Warn when # noqa: <code> contains an invalid code Mar 4, 2023
@charliermarsh charliermarsh added the suppression Related to supression of violations e.g. noqa label Mar 4, 2023
@charliermarsh charliermarsh self-assigned this Jul 5, 2023
@charliermarsh charliermarsh mentioned this issue Jul 6, 2023
Merged
charliermarsh added a commit that referenced this issue Jul 8, 2023
@charliermarsh
Emit warnings for invalid # noqa directives (#5571)
507961f 
## Summary

This PR adds a `ParseError` type to the `noqa` parsing system to enable
us to render useful warnings instead of silently failing when parsing
`noqa` codes.

For example, given `foo.py`:

```python
# ruff: noqa: x

# ruff: noqa foo

# flake8: noqa: F401
import os  # noqa: foo-bar
```

We would now output:

```console
warning: Invalid `# noqa` directive on line 2: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).
warning: Invalid `# noqa` directive on line 4: expected `:` followed by a comma-separated list of codes (e.g., `# noqa: F401, F841`).
warning: Invalid `# noqa` directive on line 6: Flake8's blanket exemption does not support exempting specific codes. To exempt specific codes, use, e.g., `# ruff: noqa: F401, F841` instead.
warning: Invalid `# noqa` directive on line 7: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).
```

There's one important behavior change here too. Right now, with Flake8,
if you do `# flake8: noqa: F401`, Flake8 treats that as equivalent to `#
flake8: noqa` -- it turns off _all_ diagnostics in the file, not just
`F401`. Historically, we respected this... but, I think it's confusing.
So we now raise a warning, and don't respect it at all. This will lead
to errors in some projects, but I'd argue that right now, those
directives are almost certainly behaving in an unintended way for users
anyway.

Closes #3339.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charliermarsh charliermarsh

Labels
suppression Related to supression of violations e.g. noqa
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Emit warnings for invalid # noqa directives astral-sh/ruff
2 participants
@charliermarsh @martinlehoux