Prefer expanding parenthesized expressions before operands#5608
Prefer expanding parenthesized expressions before operands#5608MichaReiser merged 10 commits intomainfrom
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
0cd2dde to
5dda384
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
5dda384 to
c97caf1
Compare
7135143 to
4c48749
Compare
537c84f to
56bae34
Compare
4c48749 to
4ffd817
Compare
56bae34 to
aa6915d
Compare
| ## Input | ||
|
|
||
| ```py | ||
| if True: |
There was a problem hiding this comment.
This is a regression
Ruff tests if all lines fit. This is usless in the given case because strings have no further split point. Meaning, changing the layout to fully expanded doesn't really fix the does not fit problem, because we would need to split the srings, which we cannot. Splitting the last line doesn't really help either.
Should the FitsExpanded measure mode be FitsEnd rather than StartAndEnd? Meaning, we measure if everything up to the first separator fits, then everything coming after the last line break?
There was a problem hiding this comment.
i think we do need to measure after the line break here
There was a problem hiding this comment.
I'll leave this for now to fix later. This is a general problem when using groups that contain no further split points.
8803400 to
074d289
Compare
4ffd817 to
6d11701
Compare
074d289 to
b8dff47
Compare
| let result = group(&format_args![ | ||
| if_group_breaks(&text("(")), | ||
| indent_if_group_breaks( | ||
| &format_args![soft_line_break(), format_expr], |
There was a problem hiding this comment.
why is the line break inside the indent?
There was a problem hiding this comment.
Indent is a bit counter-intuitive. Luckily, this is hidden a way by soft_block_indent and block_indent. indent only increments the indentation level, it doesn't insert any line breaks itself. The soft_line_break is necessary to then start a new line, with the new, incremented indent.
This is why the closing newline is outside of the indent. We first need to decrement the indent counter, and only then start the new line.
| ## Input | ||
|
|
||
| ```py | ||
| if True: |
There was a problem hiding this comment.
i think we do need to measure after the line break here
6d11701 to
ff18d52
Compare
b8dff47 to
830edb6
Compare
|
@MichaReiser merged this pull request with Graphite. |
Summary
This PR implements Black's behavior where it first splits off parenthesized expressions before splitting before operands to avoid unnecessary parentheses:
This is implemented by using the new IR elements introduced in #5596.
parentheses_id)conditional_groupfor the lower priority groups (all non-parenthesized expressions) with the condition that theparentheses_idgroup breaks (we want to split before operands only if the parentheses are necessary)fits_expandedto wrap all other parenthesized expressions (lists, dicts, sets), to prevent that expanding e.g. a list expands theparentheses_idgroup. We gate thefits_expandto only apply if theparentheses_idgroup fits (because we prefera\n+[b, c]over expanding[b, c]if the whole expression gets parenthesized).We limit using
fits_expandedandconditional_grouponly to expressions that themselves are not in parentheses (checking the conditions isn't free)Test Plan
It increases the Jaccard index for Django from 0.915 to 0.917
Incompatibilites
There are two incompatibilities left that I'm aware of (there may be more, I didn't go through all snapshot differences).
Long string literals
I commented on the regression. The issue is that a very long string (or any content without a split point) may not fit when only breaking the right side. The formatter than inserts the optional parentheses. But this is kind of useless because the overlong string will still not fit, because there are no new split points.
I think we should ignore this incompatibility for now
Expressions on statement level
I don't fully understand the logic behind this yet, but black doesn't break before the operators for the following example even though the expression exceeds the configured line width
But it would if the expression is used inside of a condition.
What I understand so far is that Black doesn't insert optional parentheses on the expression statement level (and a few other places) and, therefore, only breaks after opening parentheses. I propose to keep this deviation for now to avoid overlong-lines and use the compatibility report to make a decision if we should implement the same behavior.