Add ruff rules A,B,C4,EM,ISC,PERF,PGH,RET,RUF,S,SIM,SLF #549
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #549 +/- ##
=========================================
Coverage ? 95.42%
=========================================
Files ? 36
Lines ? 2450
Branches ? 0
=========================================
Hits ? 2338
Misses ? 112
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| "F", # Pyflakes | ||
| "FIX", # flake8-fixme | ||
| "FLY", # flynt | ||
| "G", # flake8-logging-format | ||
| "ICN", # flake8-import-conventions | ||
| "INP", # flake8-no-pep420 | ||
| "INT", # flake8-gettext | ||
| "ISC", # flake8-implicit-str-concat |
There was a problem hiding this comment.
The above lints look useful to me in that the changes made to make the lints pass mostly look like useful improvements. The possible exception to my mind is defining a msg variable before raising the exception - I am ambivalent to that one.
There was a problem hiding this comment.
% ruff rule EM101
Why is this bad?
By using a string literal, the error message will be duplicated in the traceback, which can make the traceback less readable.
If you would like, we can revert the EM transformations.
% ruff check --select=EM--statistics
10 EM101 [*] raw-string-in-exception
6 EM102 [*] f-string-in-exception
[*] fixable with `ruff check --fix`
| "NPY", # NumPy-specific rules | ||
| "PD", # pandas-vet | ||
| "PERF", # Perflint |
There was a problem hiding this comment.
perflint describes itself as "This project is an early beta. It will likely raise many false-positives in your code" - you have previously said that you think any and all passing lints should be included - more is necessarily better.
I am worried that new linting raises the bar for new contributors, making it harder to get started - linting has advantages and disadvantages.
If any lints should be excluded then I think this one should, given the warning from its creator.
There was a problem hiding this comment.
This pull request did not make any PERF-related changes (other than enabling PERF).
When I read the Why is this bad? section of these rule I see quantified performance gains.
Here we are not running perflint. Instead, we are running ruff which uses the original project as inspiration but rewrites each rule that the project team deems valuable in Rust. Often useless or conflicting rules are dropped. There is active discussion in the ruff repo about which rules to adopt and which to drop.
If you would like, we can disable the PERF rules.
https://docs.astral.sh/ruff/rules