Conversation
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
48afef1 to
8cd490d
Compare
|
I'm not entirely sure if I understand the Black difference that you're describing. There are two ways (besides what #5608 introduces but this hopefully isn't needed here) to control the priority
You may need to use a group sequence here (one group for the value, one group for the body) |
MichaReiser
left a comment
There was a problem hiding this comment.
Nice. Implementing list comprehension was on my TODO list for this week, I guess no more :) Would you mind commenting on #4798 next time before starting to work on a new syntax for better coordination?
| let Some(in_token) = tokens.next() else { | ||
| // Should always have an `in` | ||
| debug_assert!(false); | ||
| return CommentPlacement::Default(comment); | ||
| }; |
There was a problem hiding this comment.
Nit: We should assert that this is indeed the in token. Is there a possibility that the expression is parenthesized? If so, you may also need to skip over any ) or(.
There was a problem hiding this comment.
I'd use .expect("could not find if token in list comprehension"), i think crashing is more reasonable here than trying to format under false assumptions
There was a problem hiding this comment.
oh interesting. i though the brackets would be part of the node. til
There was a problem hiding this comment.
it's really inconsistent unfortunately (it's a particularity of our parser), e.g. tuple parentheses but others aren't are so ((1,2,3)) the range contains the inner parentheses but not the outer while in (1 + 2) they are not part of the range at all.
There was a problem hiding this comment.
find_only_token_in_range also has this problem. will fix in this pr (and then use that function)
| let Some(if_token_start) = find_if_token(locator, last_end, if_node.range().end()) else { | ||
| // there should always be an `if` here | ||
| debug_assert!(false); | ||
| return CommentPlacement::Default(comment); | ||
| }; |
There was a problem hiding this comment.
Nit: We should assert that this is indeed the expected if token and not any other token. You may also need to deal with any ( or ) in case the expression can be parenthesized.
| last_end = if_node.range().end(); | ||
| } | ||
|
|
||
| return CommentPlacement::Default(comment); |
There was a problem hiding this comment.
| return CommentPlacement::Default(comment); | |
| CommentPlacement::Default(comment) |
| [parenthesized( | ||
| "[", | ||
| &group(&self::format_args!( | ||
| &elt.format(), |
There was a problem hiding this comment.
| &elt.format(), | |
| elt.format(), |
| "[NOT_YET_IMPLEMENTED_generator_key for NOT_YET_IMPLEMENTED_generator_key in []]" | ||
| [parenthesized( | ||
| "[", | ||
| &group(&self::format_args!( |
There was a problem hiding this comment.
Question: parenthesized already adds a group. Is this additional group required or should it maybe only wrap the value rather than the value and joined? This way the printer would first try to expand joined, and only fallback to expanding value if the line still exceeds the configured line width.
There was a problem hiding this comment.
i'll check, but it might also be a mistake, parenthesized landed after i'd written this code and i added it in when i spotted it landing
There was a problem hiding this comment.
but i should also admit i don't fully grok the rules around expanding and groups yet, and sometimes end up trying a bunch of variations on examples. your explanation in a previous comment above helps a bit though (group sequences and nesting)
| let Some(in_token) = tokens.next() else { | ||
| // Should always have an `in` | ||
| debug_assert!(false); | ||
| return CommentPlacement::Default(comment); | ||
| }; |
There was a problem hiding this comment.
I'd use .expect("could not find if token in list comprehension"), i think crashing is more reasonable here than trying to format under false assumptions
| [ | ||
| i | ||
| for | ||
| i | ||
| in | ||
| [ | ||
| 1, | ||
| ] | ||
| ] |
There was a problem hiding this comment.
black formats this as
[
i
for i in [
1,
]
]which i prefer, but i expect that to be unreasonably hard to implement (breaking the inner group but not the outer group)
There was a problem hiding this comment.
Hmm interesting. I think this goes into the one case that my expression formatting refactor doesn't cover yet (and I also don't fully understand yet).
Black sometimes (for example on expression statement level) only expands by parenthesized expressions, but not by operators. But it does sometimes and inserts optional parentheses. I haven't understood the rules around this yet. It could be related and we may want to wait with addressing it just yet
476a978 to
8d11442
Compare
|
ready i think |
9e7046b to
2511815
Compare
2511815 to
7c2d38c
Compare
|
👍 I’ll have a look at the other comprehensions next |
Nice. Do you have the permissions to create an issue from #4798 and assign it to you? If not, then I'll create it and assign it to you for you, if you let me know on which comprehension you want to work first. |
|
i don't think I have any repo permissions. i guess i can create issues (not sure what you by by creating "from 4798", do you mean and link from there?) but not assign from a quick look, set and dict are pretty much like list comps so seem quite straightforward. generator comprehensions are trickier because the brackets are optional in some contexts but mandatory in others |
There's a button at the end that allows you to automatically create an issue:
I created #5674 now. I can assign it to you if you leave a comment. |
the circle with the dot in? i don't have that |
|
while there is still other work to do, maybe we can mark you can assign met to generator exp though i'll probably need a pointer on how to approach this bracket issue |
Yes. Hmm, that's unfortunate. I created the issue #5676
I'm currently looking into the expression handling. I, incorrectly, assumed that Black only uses the expand the right hand side before operands on the top level. But there seem to be other places where it applies the same logic (generators, with items, ... potentially more). You can ignore this for now. |
|
brackets: ok. note that for generator expressions it's not just a question of style, as the brackets are _required in some contexts. see e.g. https://peps.python.org/pep-0289/
|
## Summary Format `DictComp` like `ListComp` from astral-sh#5600. It's not 100%, but I figured maybe it's worth starting to explore. ## Test Plan Added ruff fixture based on `ListComp`'s.
includes
ComprehensionThe jaccard index on django goes from 0.915 to 0.923.