Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
| } | ||
|
|
||
| /// 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
left a comment
There was a problem hiding this comment.
Looks good. Putting it back into your queue to add some tests around comment handling.
| // 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
| [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.
| 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.
| 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:
| 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.
| 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.
4dd26d6 to
0613004
Compare
| [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 <micha@reiser.io>
Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Micha Reiser <micha@reiser.io>
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 <micha@reiser.io>
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.