Format expr generator exp #5804
Conversation
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
|
#5829 fixes the |
| match argument { | ||
| Expr::GeneratorExp(generator_exp) => joiner.entry( | ||
| generator_exp, | ||
| &generator_exp | ||
| .format() | ||
| .with_options(GeneratorExpParentheses::StripIfOnlyFunctionArg), | ||
| ), | ||
| other => joiner.entry(other, &other.format().with_options(parentheses)), | ||
| }; |
There was a problem hiding this comment.
Interesting, that Black has a custom logic especially for generators.
Can you add an example with a GeneratorExpr that also has comments?
len(
( # leading
a for b in c
# trailing
)
)Nit: Let's move the parentheses check from line 53..59 into the other => branch. We don't need to test if the argument is parenthesized if we're going to remove the parentheses anyway
There was a problem hiding this comment.
this isn't based on black but rather the rules for generator expressions. see e.g. "details, §2b" in the pep https://peps.python.org/pep-0289/#the-details
There was a problem hiding this comment.
note that your example will differ to black, in my understanding due to the same black bug that keeps _existing parens
There was a problem hiding this comment.
note that your example will differ to black, in my understanding due to the same black bug that keeps _existing parens
Black's behavior is to always preserve parentheses in nested expressions and I think we should do the same for generators, except if we have very good reasons not to (for consistency).
I think the idiomatic way to implement this then is to change the generator's NeedsParentheses implementation to return Never if it is the only argument, and Always otherwise (except there are other places where the parentheses should be omitted).
There was a problem hiding this comment.
note that your example will differ to black, in my understanding due to the same black bug that keeps _existing parens
Black's behavior is to always preserve parentheses in nested expressions and I think we should do the same for generators, except if we have very good reasons not to (for consistency).
is this a reference to https://github.com/astral-sh/ruff/pull/5804/files#diff-dba753754ca8da36599acf528a6bfe006404be2c4cd9d00f18db1a3cf1a7ec21R9
or https://github.com/astral-sh/ruff/pull/5804/files#diff-dba753754ca8da36599acf528a6bfe006404be2c4cd9d00f18db1a3cf1a7ec21R19
?
I think the idiomatic way to implement this then is to change the generator's
NeedsParenthesesimplementation to returnNeverif it is the only argument, andAlwaysotherwise (except there are other places where the parentheses should be omitted).
how does NeedsParentheses figure out if it's a no-sibling function argument?
There was a problem hiding this comment.
ok. i'm afraid i don't quite understand your fix suggestion. atm NeedsParentheses (afict) is only called if there is a maybe_parenthesize_expression wrapper. is that right? if so, where are you suggesting that should go? apologies if i'm missing something obvious here
There was a problem hiding this comment.
The proposal with NeedsParentheses is mainly if we want to match Black's behavior:
- Because, by default, expressions preserve their parentheses. Meaning the generators would have needed parentheses already before. Or are you aware of situations where a generator is not parenthesized in the source, but would need to be parenthesized after formatting (e.g. because we insert a trailing comma)?
- Overriding
NeedsParenthesesand returningAlwayswill add/preserve parentheses in the places where we, otherwise would omit them (e.g. in an if header).
The reason why I think that NeedsParentheses would be a good fit is, because it should inform the formatted when it is necessary to parenthesize an expression. Tuples are different, because they have their own parentheses (you can have a parenthesized tuple expression ((a, b)), but (a, b) is a tuple expression with parentheses. Okay this sounds confusing. One is about it is a parenthesized expression, the other is that tuples have their own parentheses)
There was a problem hiding this comment.
maybe i'm misunderstanding you, but in my understanding of generator expressions, they do have their "own parentheses" just like tuples do (but they can be omitted in some contexts. (fewer contexts than for tuples, only if they are a sole call argument))
maybe we merge this without preserving "double wrapped" generators and see how common when we compare ourselves against black on the various code bases we test. in general i'm in favour of matching black closer if that helps adoption
There was a problem hiding this comment.
Oh I'm sorry. That's my bad. I assumed that parenthesizing the generator is optional. But from Python's full grammar:
genexp:
| '(' ( assignment_expression | expression !':=') for_if_clauses ')'
(Oddly, the parens are not optional)
There was a problem hiding this comment.
i wonder if the function call case is covered here
primary:
| primary '.' NAME
| primary genexp <-- generator expr instead of brackets and arguments
| primary '(' [arguments] ')' <-- (regular function call)
davidszotten
force-pushed
the
format-expr-generator-exp
branch
from
49c5aec to
bcd7929
Compare
crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/generator_exp.py
Outdated
Show resolved
Hide resolved
davidszotten
force-pushed
the
format-expr-generator-exp
branch
from
bcd7929 to
2e13910
Compare
Summary
Format
GeneratorExpnote: I manually edited
generated.rssinceFormatExprGeneratorExpnow has a value and needs to be instantiated. it looks like a few others might also be in the same boat (they also break if i re-geneate)Similarity index for django: 0.978 -> 0.981
Fixes #5676
Test Plan
snapshots