Call chain formatting in fluent style #6151
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
PR Check ResultsBenchmarkLinuxWindows |
konstin
force-pushed
the
fluent_style
branch
3 times, most recently
from
9bcc65a
to
202c7f0
Compare
|
Blocked on unstable formatting of y = (
x.a() #
.b()
)
y = x.a().b() #
y = (
x.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa() #
.b()
)
y = x.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa().b() # |
konstin
force-pushed
the
fluent_style
branch
2 times, most recently
from
ee69abe
to
ca3ca2a
Compare
There was a problem hiding this comment.
Woah, nice improvement on the compatibility!
The fluent style formatting now requires to route through the fluent style in many positions (which I like more than setting in on context).
Have you considered to, instead "unroll" the call chain in the CallExpression formatting? Meaning, we would have a single formatting that owns the whole call chain formatting without calling into format attribute and format subscript (maybe parts of it). I'm asking because I find it difficult to "unroll" the recursion in my head and wonder if it would be easier if the whole call chain formatting would be in its own file.
|
|
||
| /// Switch call chain and attribute formatting to fluent style. This is otherwise identical to | ||
| /// `Never`, fluent style implies a set of outer parentheses | ||
| FluentStyle { outermost: bool }, |
There was a problem hiding this comment.
FluentStyle doesn't fit well into the Parentheses concept, which is intended to be generally applicable to all expressions.
konstin
force-pushed
the
fluent_style
branch
3 times, most recently
from
c3801b2
to
58fc27c
Compare
|
Good point, i made fluent style a bool that gets passed through the call chain formatting. It's still recursive but i think that's better than unrolling |
|
This took some rotations but now it's just a |
Could you explain your reasoning of why? |
I think consistency, mainly: Every other expression is formatted from outermost to innermost, so this is no difference. The main thing we do differently is to put a newline between the closing parentheses and the dot. |
| // Nested expressions that are in some outer expression's parentheses may be formatted in | ||
| // fluent style | ||
| let fluent_style = f.context().node_level() == NodeLevel::ParenthesizedExpression | ||
| && is_fluent_style_call_chain(expression, f.context().source()); |
There was a problem hiding this comment.
Is this still necessary here? It seems that Call, Attribute and Subscript now by-pass the Expr formatting.
There was a problem hiding this comment.
I spent way to much time trying to come up with something better, but i'm afraid the answer is yes. I think the node level is the biggest problem here because e.g. just inlining helpers doesn't seem to work.
| Expr::Attribute(expr) => { | ||
| return parenthesize_if_expands(&group(&expr.format().with_options(true))) | ||
| .fmt(f) | ||
| } | ||
| Expr::Call(expr) => { | ||
| return parenthesize_if_expands(&group(&expr.format().with_options(true))) | ||
| .fmt(f) | ||
| } | ||
| Expr::Subscript(expr) => { | ||
| return parenthesize_if_expands(&group(&expr.format().with_options(true))) | ||
| .fmt(f) | ||
| } |
There was a problem hiding this comment.
Would it be possible to remove the code from here if:
- Replace the
booleanoption with an enum that has three variants:FluentNonFluent: Left side of a call chain, but we don't use the fluent styleRoot: The root of a fluent chain (default)
CallExpressiontests if it is inside of a fluent chain if the variant isRoot. (or any node that can be the end of a call chain). If so, useFluent, otherwiseNonFluentSubscript,Call, orAttributewrap their content in agroupif the layout isFluent.- Change
can_omit_parenthesesto returnfalsefor call chains.
There was a problem hiding this comment.
I don't think i really follow; Call chains can appear about everywhere except on statement level. For the MaybeParenthesize it's special because we need to switch parentheses mode, call chain formatting always has parentheses when breaking.
Change can_omit_parentheses to return false for call chains.
This is already the case.
Unfortunately I don't understand why exactly the current solution works but the alternatives i tried don't, the interaction of node level, context, parentheses, MaybeParenthesize and FormatExpr or not is still opaque to me.
| } | ||
| _ => { | ||
| // We to format the following in fluent style: `f2 = (a).w().t(1,)` | ||
| if is_expression_parenthesized(AnyNodeRef::from(expr), source) { |
There was a problem hiding this comment.
is_expression_parenthesized returns false positives if it is the first argument in a call expression. I guess this is safe because the expression never has attributes_after_parentheses that is larger than 1.
Co-authored-by: Micha Reiser <micha@reiser.io>
|
oh i didn't turn of automerge; it should be fine anyway, i'll do a follow-up PR if anything comes up |
This looks good to me. Does this implementation handle the case you shared with me earlier correctly where it must preserve parentheses around members of the call chain? a = (
b().c(
"asdfasfaefinitive Guidddddde to Django: Web Developfdddddddment Done Rdddight"
)
).d()
a = (
b()
.c("asdfasfaefinitive Guidddddde to Django: Web Developfdddddddment Done Rdddight")
.d()
) |
|
yep it does, it's and |
Implement fluent style/call chains. See the
call_chains.pyformatting for examples.This isn't fully like black because in
raise A from Bthey allowAbreaking can influence the formatting ofBeven if it is already multiline.Similarity index:
Call chain formatting is affected by #627, but i'm cutting scope here.
Closes #5343
Test Plan: