Warn on invalid noqa even when there are no diagnostics#16178
Warn on invalid noqa even when there are no diagnostics#16178dylwil3 merged 3 commits intoastral-sh:mainfrom
Conversation
dylwil3
added
cli
|
|
It seems @charliermarsh introduced the check for performance reasons in #1898 Can you tell me a bit more: Is it possible that I'm not sure if we should skip the check if noqa is disabled because it gets initialized by the Showing warning in this case seems not in the spirit of this flag. Unless I'm misunderstanding something. |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Oh shoot. I thought you removed the entire check but you didn't. I think this looks correct. Assuming that those warnings are emitted as part of the noqa parsing logic
|
Yep they are emitted as part of the parsing of I think the UX is worth the potential perf hit (I also think the situation you'd have to be in for the perf hit to be noticeable seems somewhat rare - something like a cold start on a large codebase with no lint errors and lots of comments?) But I can try to run some kind of benchmark tomorrow to double check. |
|
This seems okay to me... and again, I think it's a relatively rare situation. After deleting files with syntax errors, I found a code that had no diagnostics ( (Also, the absolute numbers below are probably misleading since I didn't do any work to make sure my CPU was idle etc. So it's only the relative difference that's of interest). ❯ hyperfine --warmup 10 \
"./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated --no-cache -e" "ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated --no-cache -e"
Benchmark 1: ./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated --no-cache -e
Time (mean ± σ): 78.9 ms ± 1.6 ms [User: 648.6 ms, System: 89.3 ms]
Range (min … max): 76.3 ms … 82.9 ms 36 runs
Benchmark 2: ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated --no-cache -e
Time (mean ± σ): 78.5 ms ± 1.5 ms [User: 644.1 ms, System: 87.9 ms]
Range (min … max): 75.9 ms … 82.3 ms 37 runs
Summary
ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated --no-cache -e ran
1.00 ± 0.03 times faster than ./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated --no-cache -eAnd with a warm cache it's even more of a non-issue: ❯ hyperfine --warmup 10 \
"./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated -e" "ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated -e"
Benchmark 1: ./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated -e
Time (mean ± σ): 15.3 ms ± 1.0 ms [User: 21.4 ms, System: 46.2 ms]
Range (min … max): 13.3 ms … 18.1 ms 167 runs
Benchmark 2: ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated -e
Time (mean ± σ): 15.1 ms ± 0.9 ms [User: 21.8 ms, System: 46.2 ms]
Range (min … max): 13.0 ms … 18.0 ms 167 runs
Summary
ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated -e ran
1.01 ± 0.09 times faster than ./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ --select YTT101 --isolated -e |
2 participants
On
mainwe warn the user if there is an invalid noqa comment1 and at least one of the following holds:noqas is enabled (e.g.RUF100)This is probably strange behavior from the point of view of the user, so we now show invalid
noqas even when there are no diagnostics.Closes #12831
Footnotes
For the current definition of "invalid noqa comment", which may be expanded in Warn on invalid
# noqarule codes #12811 . This PR is independent of loc. cit. in the sense that the CLI warnings should be consistent, regardless of whichnoqacomments are considered invalid. ↩