Correctly handle left/right breaking of binary expression #4985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Merged
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
MichaReiser
commented
Jun 9, 2023
Comment on lines
46
to
75
| BinaryLayout::Default => { | ||
| let comments = f.context().comments().clone(); | ||
| let operator_comments = comments.dangling_comments(item.as_any_node_ref()); | ||
| let needs_space = !is_simple_power_expression(item); | ||
|
|
||
| let before_operator_space = if needs_space { | ||
| soft_line_break_or_space() | ||
| } else { | ||
| soft_line_break() | ||
| }; | ||
|
|
||
| write!( | ||
| f, | ||
| [ | ||
| left.format(), | ||
| before_operator_space, | ||
| op.format(), | ||
| trailing_comments(operator_comments), | ||
| ] | ||
| )?; | ||
|
|
||
| // Format the operator on its own line if the right side has any leading comments. | ||
| if comments.has_leading_comments(right.as_ref()) { | ||
| write!(f, [hard_line_break()])?; | ||
| } else if needs_space { | ||
| write!(f, [space()])?; | ||
| } | ||
|
|
||
| write!(f, [group(&right.format())]) | ||
| } |
Comment on lines
128
to
141
| write!( | ||
| f, | ||
| [ | ||
| // Wrap the left in a group and gives it an id. The printer first breaks the | ||
| // right side if `right` contains any line break because the printer breaks | ||
| // sequences of groups from right to left. | ||
| // Indents the left side if the group breaks. | ||
| group(&format_args![ | ||
| if_group_breaks(&text("(")), | ||
| indent_if_group_breaks( | ||
| &format_args![ | ||
| soft_line_break(), | ||
| left.format(), | ||
| soft_line_break_or_space(), | ||
| op.format(), | ||
| space() | ||
| ], | ||
| left_group | ||
| ) | ||
| ]) | ||
| .with_group_id(Some(left_group)), | ||
| // Wrap the right in a group and indents its content but only if the left side breaks | ||
| group(&indent_if_group_breaks(&right.format(), left_group)), | ||
| // If the left side breaks, insert a hard line break to finish the indent and close the open paren. | ||
| if_group_breaks(&format_args![hard_line_break(), text(")")]) | ||
| .with_group_id(Some(left_group)) | ||
| ] | ||
| ) | ||
| } |
MichaReiser
commented
Jun 9, 2023
a315ff0 to
5243b37
Compare
MichaReiser
commented
Jun 19, 2023
MichaReiser
commented
Jun 19, 2023
439c585 to
a39dded
Compare
a39dded to
d785d44
Compare
Merged
MichaReiser
commented
Jun 19, 2023
f672744 to
89c5ea6
Compare
d785d44 to
5c30e73
Compare
This was referenced Jun 20, 2023
Merged
d6272b0 to
d90460d
Compare
5c30e73 to
516784c
Compare
konstin
approved these changes
Jun 20, 2023
| text("("), | ||
| block_indent(&format_args![ | ||
| left, | ||
| hard_line_break(), |
There was a problem hiding this comment.
This formatting always looks kinda odd to me but i guess python doesn't allow much better
| // Wrap the left in a group and gives it an id. The printer first breaks the | ||
| // right side if `right` contains any line break because the printer breaks | ||
| // sequences of groups from right to left. | ||
| // Indents the left side if the group breaks. |
There was a problem hiding this comment.
i'm having trouble following this code, shouldn't right breaks and if right is expanded but this is insufficient the whole thing should be parenthesized and indented?
There was a problem hiding this comment.
I'm not entirely sure if I understand this comment.
The formatter measures the left group to see if it fits. It does that by measuring if all the content up to the first line break (soft or hard) in the right group (or whatever comes after) fits on a line. It doesn't expand the left group if that's the case.
This is is the behavior that we want. We want to expand the right first and not add parentheses in that case. We only want to add parentheses and indent the code if the left expands. That's why indent_if_group_breaks and the if_group_breaks refer to the left group and not their enclosing group.
516784c to
c99b89b
Compare
|
@MichaReiser merged this pull request with Graphite. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Black supports for layouts when it comes to breaking binary expressions:
Our current implementation only handles
ExpandRightandDefaultcorrectly. This PR adds support forExpandRightThenLeftandExpandLeft.Test Plan
I added tests that play through all 4 binary expression layouts.