Add rule and autofix to sort the contents of __all__
#9474
Conversation
AlexWaygood
force-pushed
the
sort-dunder-all-2
branch
from
fd1de34 to
5bacac0
Compare
|
Some miscellaneous notes:
|
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF022 | 156 | 156 | 0 | 0 | 0 |
|
Oh, one other note: I decided to implement this as an RUF rule, since:
Since starting working on the rule, however, I realised that the https://github.com/cpendery/asort tool does also exist, so possibly we could call this rule ASORT001 instead of RUF022. I don't have a strong opinion either way! |
CodSpeed Performance ReportMerging #9474 will degrade performances by 4.36%Comparing Summary
Benchmarks breakdown
|
|
Thanks for the extremely helpful review @BurntSushi! |
| Applicability::Unsafe | ||
| } else { | ||
| Applicability::Safe | ||
| } |
There was a problem hiding this comment.
I tend to think that using unsafe to protect comments is a bit of a misuse of the concept, which is meant to protect against possible breakages via changes in semantics. But, this question has come up before, and I don't know that we have a unified answer.
\cc @zanieb
There was a problem hiding this comment.
I feel like as a user of ruff I'd probably be a little annoyed if ruff did something like #9474 (comment) to my carefully designed __all__ definition, and then claimed it was a "safe" fix 😄 but I'll defer to you and Zanie :)
There was a problem hiding this comment.
That's a fair point. We would likely need to mark all fixes as unsafe following that reasoning because:
- some fixes don't handle comments at all
- Many fixes don't produce nicely formatted code. You could use the same argument that we should use unsafe because ruff could mess up my carefully hand-formatted code.
Let's see what @zanieb thinks
There was a problem hiding this comment.
I've pushed 24616f3 to mark the fix as always being safe, but I've kept a note in the docs stating that it might sometimes move comments to unexpected locations
|
Okay, just pushed some major updates:
|
|
Here's the latest diff when running this autofix on CPython, FWIW: cpython_diff.txt |
There was a problem hiding this comment.
Overall this looks good to me, but there are a few clarification that are needed for me to understand the code and:
- We should align the
PartialEqandOrdimplementation ofAllDunderItems - Let's add some tests around unparenthesized tuples that start with a parenthesized first item (see inline comment). I suspect that things might go wrong in that case
It would be interesting to explore if we can implement the check as an AST rule and only use lexing for generating the fix. I'll leave that up to you if you want to tackle this and if so, if you want to do it as part of this PR.
… into sort-dunder-all-2
|
Thanks all for the reviews! |
Summary
This implements the rule proposed in #1198 (though it doesn't close the issue, as there are some open questions about configuration that might merit some further discussion). This ended up being a bigger PR than I expected! I've tried to make sure it's well commented, so hopefully it's not too hard to figure out what's going on.
Test Plan
cargo test/cargo insta review. I also ran this rule on the CPython codebase, and it produces this diff, which looks pretty good to me: cpython_diff.txt