Conversation
Summary -- This PR incorporates some feedback from internal discussions on [Discord] and from #23203 into the preview default rule set. In particular, we drop: - [`PERF401`](https://docs.astral.sh/ruff/rules/PERF401) - [`PERF403`](https://docs.astral.sh/ruff/rules/PERF403) - [`PLR1714`](https://docs.astral.sh/ruff/rules/PLR1714) - [`RET504`](https://docs.astral.sh/ruff/rules/RET504) - [`SIM102`](https://docs.astral.sh/ruff/rules/SIM102) - [`SIM114`](https://docs.astral.sh/ruff/rules/SIM114) - [`TRY300`](https://docs.astral.sh/ruff/rules/TRY300) A couple of these disagree a bit with Clippy's own categorization. For example, our SIM102 is [collapsible_if], SIM114 is [if_same_then_else], and RET504 is [let_and_return], all of which are Style lints for Clippy. But if users find them annoying in Python, it makes sense to me to bump them down to Pedantic. PERF401 and PERF403 also have open issues (#21890, #21891), so I think it makes sense to consider them lower severity, at least until those are resolved. TRY300 and PLR1714 are just a bit more pedantic than expected and some people prefer the styles they flag. [Discord]: https://discord.com/channels/1039017663004942429/1082324250112823306/1478345981916348526 [let_and_return]: https://rust-lang.github.io/rust-clippy/master/?search=assign#let_and_return [collapsible_if]: https://rust-lang.github.io/rust-clippy/master/?search=collap#collapsible_if [if_same_then_else]: https://rust-lang.github.io/rust-clippy/master/?search=then_#if_same_then_else Test Plan -- Future default users
these are based on internal adoption feedback from: https://discord.com/channels/1039017663004942429/1082324250112823306/1478345981916348526
this came up both internally and in the discussion, and it feels similar in spirit to SIM114
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RET504 | 457 | 0 | 457 | 0 | 0 |
| SIM102 | 178 | 0 | 178 | 0 | 0 |
| TRY300 | 118 | 0 | 118 | 0 | 0 |
| PERF401 | 107 | 0 | 107 | 0 | 0 |
| PLR1714 | 36 | 0 | 36 | 0 | 0 |
| SIM114 | 31 | 0 | 31 | 0 | 0 |
| PERF403 | 5 | 0 | 5 | 0 | 0 |
|
I'll personally be a tiny bit sad if SIM102 and SIM114 don't make it into the defaults. They seem like easy wins for both readability and simplicity. |
Yeah I'm a bit torn on these too. I think they're nice in simple cases like the ones in the docs, but I just ignored a Clippy lint for SIM114 today when I wanted the same code in each branch, and I also found some of the real examples on Discord clearly less readable. For example: -if something in [value1, value2] and another > condition:
- do_something()
-elif something_else == "a string" and condition:
+if something in [value1, value2] and another > condition or something_else == "a string" and condition:
do_something()Now I have to think about precedence, and it gets reformatted like this: if (
something in [value1, value2]
and another > condition
or something_else == "a string"
and condition
):
do_something()I don't feel super strongly, but they have come up multiple times from different sources. |
|
Wow, that suggested change should really come with wrapping parens to clarify OoO and where it previously came from, because that precedence is rough. |
Summary
This PR incorporates some feedback from internal discussions on Discord and
from #23203 into the preview default rule set. In particular, we drop:
PERF401PERF403PLR1714RET504SIM102SIM114TRY300A couple of these disagree a bit with Clippy's own categorization. For example,
our SIM102 is collapsible_if, SIM114 is if_same_then_else, and RET504 is
let_and_return, all of which are Style lints for Clippy. But if users find
them annoying in Python, it makes sense to me to bump them down to Pedantic.
PERF401 and PERF403 also have open issues (#21890, #21891), so I think it makes
sense to consider them lower severity, at least until those are resolved.
TRY300 and PLR1714 are just a bit more pedantic than expected and some people
prefer the styles they flag.
Test Plan
Future default users