Fix format named expr comments #5705

davidszotten · 2023-07-12T08:50:44Z

davidszotten · 2023-07-12T08:53:00Z

crates/ruff_python_formatter/src/expression/expr_named_expr.rs

+        let leading_value_comments = comments.leading_comments(value.as_ref());
+        let dangling_item_comments = comments.dangling_comments(item);
+
+        if trailing_target_comments.is_empty() {


i tried making a single write! call with these ifs inside, but that doesn't work since space() and soft_line_break have different types. is there some trick to make this work? (or do we prefer multiple write calls?

There's format_with but personally i like this style better, multiple write calls are fine here, if required we can still optimize later

konstin · 2023-07-12T08:58:04Z

crates/ruff_python_formatter/src/expression/expr_named_expr.rs

+        let leading_value_comments = comments.leading_comments(value.as_ref());
+        let dangling_item_comments = comments.dangling_comments(item);
+
+        if trailing_target_comments.is_empty() {


There's format_with but personally i like this style better, multiple write calls are fine here, if required we can still optimize later

konstin · 2023-07-12T08:59:39Z

crates/ruff_python_formatter/src/expression/expr_named_expr.rs

+        if trailing_target_comments.is_empty() {
+            write!(f, [space()])?;
+        } else {
+            write!(f, [soft_line_break()])?;


What's the reason for this being a soft line break?

as opposed to?

a hard_line_break, since as far as i can tell we always want to break after the comments if there are any. I realized it doesn't make a difference here because a trailing comment will expand the group, but it might be easier to read

konstin · 2023-07-12T09:00:50Z

crates/ruff_python_formatter/src/expression/expr_named_expr.rs

+        write!(f, [text(":="), dangling_comments(dangling_item_comments)])?;
+
+        if leading_value_comments.is_empty() {
+            write!(f, [space()])?;


What happens when there are dangling_item_comments but no leading leading_value_comments? this would make a good test case

good catch. definitely something not what i expected. will continue investigating

konstin · 2023-07-12T09:01:46Z

crates/ruff_python_formatter/src/trivia.rs

@@ -185,6 +185,10 @@ pub(crate) enum TokenKind {
    /// `.`.
    Dot,

+    /// `:=`.
+    /// TODO: name?


walrus is correct

github-actions · 2023-07-12T09:05:07Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.4±0.01ms     4.9 MB/sec    1.01      8.5±0.01ms     4.8 MB/sec
formatter/numpy/ctypeslib.py               1.00   1899.9±2.13µs     8.8 MB/sec    1.01   1917.8±4.14µs     8.7 MB/sec
formatter/numpy/globals.py                 1.00    204.7±0.37µs    14.4 MB/sec    1.01    207.1±0.27µs    14.2 MB/sec
formatter/pydantic/types.py                1.00      4.2±0.01ms     6.0 MB/sec    1.00      4.2±0.01ms     6.0 MB/sec
linter/all-rules/large/dataset.py          1.00     14.0±0.03ms     2.9 MB/sec    1.00     14.1±0.02ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.00ms     4.7 MB/sec    1.00      3.5±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    364.2±1.05µs     8.1 MB/sec    1.00    364.0±1.13µs     8.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.2±0.01ms     4.1 MB/sec    1.00      6.2±0.04ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.01      7.2±0.02ms     5.7 MB/sec    1.00      7.1±0.01ms     5.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1465.7±2.64µs    11.4 MB/sec    1.00   1452.3±2.85µs    11.5 MB/sec
linter/default-rules/numpy/globals.py      1.01    156.4±0.23µs    18.9 MB/sec    1.00    155.4±0.66µs    19.0 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.2±0.00ms     8.0 MB/sec    1.00      3.2±0.04ms     8.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     11.0±0.45ms     3.7 MB/sec    1.10     12.2±0.67ms     3.3 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.5±0.13ms     6.8 MB/sec    1.09      2.7±0.13ms     6.2 MB/sec
formatter/numpy/globals.py                 1.00   289.3±16.98µs    10.2 MB/sec    1.01   291.9±24.53µs    10.1 MB/sec
formatter/pydantic/types.py                1.00      5.7±0.28ms     4.5 MB/sec    1.01      5.8±0.30ms     4.4 MB/sec
linter/all-rules/large/dataset.py          1.04     18.1±1.51ms     2.2 MB/sec    1.00     17.4±0.44ms     2.3 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.5±0.39ms     3.7 MB/sec    1.04      4.7±0.26ms     3.5 MB/sec
linter/all-rules/numpy/globals.py          1.00   509.4±19.13µs     5.8 MB/sec    1.17   596.6±40.30µs     4.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.5±0.36ms     3.4 MB/sec    1.10      8.3±0.33ms     3.1 MB/sec
linter/default-rules/large/dataset.py      1.00      9.2±0.74ms     4.4 MB/sec    1.03      9.5±0.35ms     4.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02  1969.2±76.88µs     8.5 MB/sec    1.00  1932.5±84.95µs     8.6 MB/sec
linter/default-rules/numpy/globals.py      1.01   233.4±14.19µs    12.6 MB/sec    1.00   230.1±13.11µs    12.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.0±0.13ms     6.4 MB/sec    1.01      4.0±0.19ms     6.4 MB/sec

MichaReiser · 2023-07-12T11:42:59Z

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/named_expr.py

-    pass
+    ...
+
+# should not add brackets


Hey. Just wanted to let you know that I'm working on changing NeedsParentheses to pass through the parent node. This should make it straightforward to implement the correct parentheses logic for the walrus operator.

👍

atm i can't even figure out what's causing these parens to be added. parenthesize gets set to "ifbreaks" and nothing is breaking...

This PR ships the infrastructure: #5708

The parentheses are added because FormatExpr first calls into NeedsParentheses to determine if parentheses are necessary

ruff/crates/ruff_python_formatter/src/expression/mod.rs

Line 65 in 33a9177

let parentheses = item.needs_parentheses(self.parenthesize, f.context());

And the implementation of NamedExpr returns Always

ruff/crates/ruff_python_formatter/src/expression/expr_named_expr.rs

Lines 39 to 44 in 33a9177

match default_expression_needs_parentheses(self.into(), parenthesize, context) {

// Unlike tuples, named expression parentheses are not part of the range even when

// mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details.

Parentheses::Optional => Parentheses::Always,

parentheses => parentheses,

}

We should implement the same rules as in https://github.com/psf/black/blob/d1248ca9beaf0ba526d265f4108836d89cf551b7/src/black/linegen.py#L1381-L1395 (requires access to the parent node)

aah. thanks

do i understand correctly that his is separate to (i.e. won't help with) parens for generator comprehensions (that we were discussing yesterday)?

That's correct. The problem is related but needs a separate fix (and I first need to figure out what even the expected output is... I have suspicion I know how it's supposed to behave but I haven't verified it yet)

charliermarsh · 2023-08-23T22:36:03Z

I suspect this can now be closed as the originating issue is resolved.

add TokenKind::Walrus

c89eab9

davidszotten commented Jul 12, 2023

View reviewed changes

davidszotten added 2 commits July 12, 2023 09:54

handle comments around walrus op

06fef9f

also handle end of line comments trailing a lone walrus

982b712

davidszotten force-pushed the fix-format-named-expr-comments branch from e707349 to 982b712 Compare July 12, 2023 08:54

konstin approved these changes Jul 12, 2023

View reviewed changes

konstin requested a review from MichaReiser July 12, 2023 09:02

MichaReiser reviewed Jul 12, 2023

View reviewed changes

konstin added the formatter Related to the formatter label Jul 16, 2023

MichaReiser closed this Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix format named expr comments #5705

Fix format named expr comments #5705

davidszotten commented Jul 12, 2023

davidszotten Jul 12, 2023

konstin Jul 12, 2023

konstin Jul 12, 2023

konstin Jul 12, 2023

davidszotten Jul 12, 2023

konstin Jul 12, 2023

konstin Jul 12, 2023

davidszotten Jul 12, 2023

konstin Jul 12, 2023

github-actions bot commented Jul 12, 2023 •

edited

Loading

MichaReiser Jul 12, 2023

davidszotten Jul 12, 2023

MichaReiser Jul 12, 2023

davidszotten Jul 12, 2023

MichaReiser Jul 12, 2023

charliermarsh commented Aug 23, 2023

	match default_expression_needs_parentheses(self.into(), parenthesize, context) {
	// Unlike tuples, named expression parentheses are not part of the range even when
	// mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details.
	Parentheses::Optional => Parentheses::Always,
	parentheses => parentheses,
	}

Fix format named expr comments #5705

Fix format named expr comments #5705

Conversation

davidszotten commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 12, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 23, 2023

github-actions bot commented Jul 12, 2023 •

edited

Loading