Add a rule/autofix to sort __slots__ and __match_args__
#9564
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF023 | 150 | 150 | 0 | 0 | 0 |
AlexWaygood
force-pushed
the
sort-dunder-slots
branch
from
23cfdbc to
39a059b
Compare
AlexWaygood
changed the title
__match_args____match_args__
|
Okay, after the latest push, we now fix all but one violation in CPython (there's only one place in CPython where a multiline dict is used for |
CodSpeed Performance ReportMerging #9564 will not alter performanceComparing Summary
|
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct NodeAnalysis<'a> { |
There was a problem hiding this comment.
Nit: The name here isn't telling me much. What kind of analysis is it? Is it like the SortDunderAnalysis? Or can we give that structure another name, because the structure isn't the analysis. What does it represent?
There was a problem hiding this comment.
Pushed a rename and some more docs in cf6ca75 -- any better?
I really struggled with naming this the other day, but coming back to it after a few days made it seem obvious :)
|
I'm really astonished how quickly you closed #1198 and brought to live this follow-up PR - big kudos!
Just wanted to let you know that I'd be very interested in autofixes for multline tuple- |
Have no fear, the description in my PR summary is already slightly out of date — I've already implemented the autofix for multiline tuple |
| } | ||
| } | ||
|
|
||
| fn create_fix( |
There was a problem hiding this comment.
Should this be a method on StringLiteralDisplay?
There was a problem hiding this comment.
Not sure... it makes the signature cleaner, but I was thinking of this struct as just a pretty simple container for heterogenous data; this breaks my mental model of that slightly. Anyway, I implemented it as a method in e64879a :)
| } | ||
| } | ||
|
|
||
| impl Eq for IsortSortKey<'_> {} |
There was a problem hiding this comment.
@BurntSushi - Do these Ord / PartialOrd / PartialEq / Eq implementations look right / consistent to you? (I've never properly learned the relationships between them.)
There was a problem hiding this comment.
(FWIW: these haven't changed in this PR, just moved to a different file, and Micha reviewed these in my previous PR (my initial implementations in my previous PR had some issues, which Micha pointed out))
| where | ||
| F: FnMut(&str, &str) -> Ordering, | ||
| { | ||
| assert_eq!(elts.len(), elements.len()); |
There was a problem hiding this comment.
I think he's suggesting creating a dedicated struct like that. Since you're allocating below anyway with collect_vec, perhaps you want to do something like:
struct Whatever<'a>(Vec<(&'a Expr, &'a str)>)That is: store the zipped elements, so that the constraint is always enforced once you've created the struct.
| } | ||
| } | ||
|
|
||
| /// Collect data on each line of a multiline string sequence. |
There was a problem hiding this comment.
How much of the below is new code vs. moved from the isort rule?
There was a problem hiding this comment.
I think everything below this point is entirely unchanged, just moved, except that a few things have been renamed and a few comments tweaked (e.g. collect_dunder_all_lines() has been renamed to collect_string_sequence_lines(), etc.)
…ring literals after a string literal that uses implicit concatenation
AlexWaygood
changed the title
__match_args____slots__ and __match_args__
| // Safe from underflow, as the constructor guarantees | ||
| // that the underlying vector has length >= 2 | ||
| self.0.len() - 1 | ||
| } |
There was a problem hiding this comment.
I feel like I'd find it clearer to just have .len() on this and do if i < element_trios.len() - 1, since that's such a common pattern in iteration.
There was a problem hiding this comment.
I definitely know what you mean, but here specifically I do quite like having this logic right next to the assertions in the constructor method that maintain the invariant that protect us from underflow here
AlexWaygood
force-pushed
the
sort-dunder-slots
branch
from
d5606fa to
333fe62
Compare
|
Thanks both for the reviews! :D I've had two approvals, so I'm landing this now, but very happy to tackle any further comments in followup PRs :) |
Summary
This PR introduces a new rule to sort
__slots__and__match_args__according to a natural sort, as was requested in #1198 (comment).The implementation here generalises some of the machinery introduced in 3aae16f so that different kinds of sorts can be applied to lists of string literals. (We use an "isort-style" sort for
__all__, but that isn't really appropriate for__slots__and__match_args__, where nearly all items will be snake_case.) Several sections of code have been moved fromsort_dunder_all.rsto a new module,sorting_helpers.rs, whichsort_dunder_all.rsandsort_dunder_slots.rsboth make use of.__match_args__is very similar to__all__, in that it can only be a tuple or a list.__slots__differs from the other two, however, in that it can be any iterable of strings. If slots is a dictionary, the values are used by the builtinhelp()function as per-attribute docstrings that show up in the output ofhelp(). (There's no particular use-case for making__slots__a set, but it's perfectly legal at runtime, so there's no reason for us not to handle it in this rule.)Currently I've only implemented an autofix for
__match_args__and__slots__if the definition is on a single line. That's for two reasons:__slots__and__match_args__definitions are much rarer than multiline__all__definitions, so there's probably less value in implementing the autofix for those.Implementing an autofix for multiline
__match_args__wouldn't be too hard: I'd just have to move more code fromsort_dunder_all.rstosorting_helpers.rs. The same is true for__slots__when it's a tuple, list or set. I don't feel enthused about implementing an autofix for multiline__slots__definitions where__slots__is a dict, however. That would require a lot of extra code, and using a dict for__slots__is pretty rare.If we're interested in autofixes for multiline
__slots__and__match_args__definitions, let me know whether you'd prefer it in this PR or a followup!Test Plan
cargo test/cargo insta review.I also ran this rule on CPython. The autofix fixed 45 violations:
Diff from the autofix
The following 17 violations were flagged but left unfixed by the rule as it currently stands. (They were unfixed because they're multiline definitions):