Format ExprTuple #4963
Merged
Format ExprTuple #4963
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. |
konstin
commented
Jun 8, 2023
| } | ||
| } | ||
|
|
||
| /// Check if a tuple has already had parentheses in the input | ||
| fn is_parenthesized( |
There was a problem hiding this comment.
not sure if this would be useful anywhere else
There was a problem hiding this comment.
It's good to know that it exists. We can pull it out if it is used elsewhere.
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
MichaReiser
requested changes
Jun 8, 2023
| // Handle the edge cases of an empty tuple and a tuple with one element | ||
| let last = match &elts[..] { | ||
| [] => { | ||
| return text("()").fmt(f); |
There was a problem hiding this comment.
Can you add a test for an empty tuple that contains a comment:
(
# empty
)You'll need to handle that case manually by:
- overriding
fmt_dangling_commentsonFormatNodeRule - Use
block_indent(&dangling_node_comments(item.into())here to format the dangling comments proper
There was a problem hiding this comment.
didn't realize earlier, but it's neat that block_ident is a noop when empty
| [group(&format_args![ | ||
| // A single element tuple always needs parentheses | ||
| &text("("), | ||
| soft_block_indent(&group(&format_args![single.format(), &text(",")])), |
crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/tuple_expression.py
Show resolved
Hide resolved
|
|
||
| let saved_level = f.context().node_level(); | ||
| // Tell the children they need parentheses | ||
| f.context_mut().set_node_level(NodeLevel::Expression); |
There was a problem hiding this comment.
Shouldn't this not already always be true? Because the Expr formatting sets the level to NodeLevel::Expression
There was a problem hiding this comment.
yep, forgot to update that code
Comment on lines
62
to
69
| [group(&format_args![ | ||
| // An expanded group always needs parentheses | ||
| &text("("), | ||
| hard_line_break(), | ||
| block_indent(&ExprSequence::new(elts)), | ||
| hard_line_break(), | ||
| &text(")"), | ||
| ])] |
There was a problem hiding this comment.
The group here is useless because you'll always force it to expand by writing the block_indent and the hard_line_break.
Comment on lines
116
to
124
| if pos == self.elts.len() - 1 { | ||
| write!( | ||
| f, | ||
| [ | ||
| entry.format(), | ||
| if_group_breaks(&text(",")), | ||
| soft_line_break() | ||
| ] | ||
| )?; |
There was a problem hiding this comment.
Nit: You can simplify this by always writing the trailing comma after having written all elements.
Comment on lines
113
to
127
| for (pos, entry) in self.elts.iter().enumerate() { | ||
| // We need a trailing comma on the last entry of an expanded group since we have more | ||
| // than one element | ||
| if pos == self.elts.len() - 1 { | ||
| write!( | ||
| f, | ||
| [ | ||
| entry.format(), | ||
| if_group_breaks(&text(",")), | ||
| soft_line_break() | ||
| ] | ||
| )?; | ||
| } else { | ||
| write!(f, [entry.format(), text(","), soft_line_break_or_space()])?; | ||
| } |
There was a problem hiding this comment.
You can simplify this by using joiner:
Suggested change
| for (pos, entry) in self.elts.iter().enumerate() { | |
| // We need a trailing comma on the last entry of an expanded group since we have more | |
| // than one element | |
| if pos == self.elts.len() - 1 { | |
| write!( | |
| f, | |
| [ | |
| entry.format(), | |
| if_group_breaks(&text(",")), | |
| soft_line_break() | |
| ] | |
| )?; | |
| } else { | |
| write!(f, [entry.format(), text(","), soft_line_break_or_space()])?; | |
| } | |
| let separator = format_with(|f| write!(f[text(","), soft_line_break_or_space()])); | |
| f.join_with(separator).entries(self.elts.iter().formatted()).finish()?; | |
| // Write the trailing comma | |
| write!(f, [if_group_breaks(&text(","))])?; |
| } | ||
| } | ||
|
|
||
| /// Check if a tuple has already had parentheses in the input | ||
| fn is_parenthesized( |
There was a problem hiding this comment.
It's good to know that it exists. We can pull it out if it is used elsewhere.
| } | ||
| } | ||
|
|
||
| /// Check if a tuple has already had parentheses in the input | ||
| fn is_parenthesized( | ||
| range: TextRange, |
There was a problem hiding this comment.
Suggested change
| range: TextRange, | |
| tuple_range: TextRange, |
I had to look it up on the call side to know what range this is.It could even make sense to pass the whole tuple
| f: &mut Formatter<PyFormatContext<'_>>, | ||
| ) -> bool { | ||
| let parentheses = "("; | ||
| let first_char = &f.context().contents()[TextRange::at(range.start(), parentheses.text_len())]; |
There was a problem hiding this comment.
I believe this panics if the first character is a non-ascii character.
It's probably safe to use `&f.context().contents(usize::from(range.start())..].chars().next()
This implements formatting ExprTuple, including magic trailing comma. I intentionally didn't change the settings mechanism but just added a dummy global const flag. Besides the snapshots, I added custom breaking/joining tests and a deeply nested test case. The diffs look better than previously, proper black compatibility depends on parentheses handling.
konstin
force-pushed
the
format-expr-tupl-try-2
branch
from
4dd26d6 to
0613004
Compare
MichaReiser
approved these changes
Jun 9, 2023
Comment on lines
35
to
40
| [group(&format_args![ | ||
| // A single element tuple always needs parentheses | ||
| &text("("), | ||
| block_indent(&dangling_node_comments(item)), | ||
| &text(")"), | ||
| ])] |
There was a problem hiding this comment.
It's not necessary to use a group here because the block_indent emits a hard_line_break that always forces the group to expand
Co-authored-by: Micha Reiser <[email protected]>
Co-authored-by: Micha Reiser <[email protected]>
Co-authored-by: Micha Reiser <[email protected]>
72 tasks
konstin
added a commit
that referenced
this pull request
Jun 13, 2023
This implements formatting ExprTuple, including magic trailing comma. I intentionally didn't change the settings mechanism but just added a dummy global const flag. Besides the snapshots, I added custom breaking/joining tests and a deeply nested test case. The diffs look better than previously, proper black compatibility depends on parentheses handling. --------- Co-authored-by: Micha Reiser <[email protected]>
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.
This implements formatting ExprTuple, including magic trailing comma. I intentionally didn't change the settings mechanism but just added a dummy global const flag.
Besides the snapshots, I added custom breaking/joining tests and a deeply nested test case. The diffs look better than previously, proper black compatibility depends on parentheses handling.