Enforce a few more ruff rules#12157
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 9 files ±0 9 suites ±0 3h 11m 18s ⏱️ -42s Results for commit 34b43a2. ± Comparison against base commit e867681. ♻️ This comment has been updated with latest results. |
jacobtomlinson
left a comment
There was a problem hiding this comment.
I think I'm -1 on merging this. It's clear that we aren't running into any of these problems because there are no fixes needed. I expect because they aren't relevant or are being handled by other tools/linters/rules.
Adding more rules for the sake of it just makes things more likely to run into rule conflict in the future. We should only add things if they add value.
| "B", | ||
| "T10", | ||
| "TID", | ||
| "ISC", |
There was a problem hiding this comment.
I expect black is fixing these for us already.
There was a problem hiding this comment.
Older versions of black would actually create these when joining split lines. But yes, recent versions do seem to join split lines correctly.
However, I am not certain black fixes occurrences from older versions, or manually created occurrences, which is why I tend to add these rules. Ruff itself is unable to autofix some occurrences, I guess it's not an easy task.
adf7781 to
780b503
Compare
This is (or at least used to be) useful to catch cases where black would reformat: "line1" "line2" as: "line1" "line2" instead of: "line1line2"
This should be a replaceemnt for this discarded isort option: add_imports = ["from __future__ import annotations"] See 1ab0d43.
I believe the maintainers do have an interest in rule TID252, since ruff doesn't raise a single issue in this project: Absolute imports, or relative imports from siblings, are recommended by PEP 8 [...]
This reverts commit b316cf2.
e4e1204 to
59c1bd6
Compare
Order rules according to the ruff documentation: https://docs.astral.sh/ruff/rules/
See comments in individual commits. Enabling these rules does't raise any issue. This is a kind of proof that they apply well to this project 😄