Comments outside expression parentheses #7873

konstin · 2023-10-09T16:56:40Z

Summary

I've removed automatic dangling comment formatting, we're doing manual dangling comment formatting everywhere anyway (the assert-all-comments-formatted ensures this) and dangling comments would break the formatting there.

main:

project	similarity index	total files	changed files
cpython	0.75803	1799	1647
django	0.99983	2772	35
home-assistant	0.99953	10596	189
poetry	0.99891	317	17
transformers	0.99963	2657	332
twine	1.00000	33	0
typeshed	0.99978	3669	20
warehouse	0.99969	654	15
zulip	0.99970	1459	22

PR:

project	similarity index	total files	changed files
cpython	0.75803	1799	1647
django	0.99983	2772	34
home-assistant	0.99953	10596	186
poetry	0.99891	317	17
transformers	0.99966	2657	330
twine	1.00000	33	0
typeshed	0.99978	3669	20
warehouse	0.99977	654	13
zulip	0.99970	1459	22

Test Plan

New test file.

konstin · 2023-10-12T15:05:49Z

An alternative we could explore is storing the parentheses range on OptionalParentheses through needs_parentheses

MichaReiser

How does this work for expressions where we use maybe_parenthesize with a layout that doesn't preserve parentheses and a trailing (non-own line) comment?

crates/ruff_python_formatter/src/comments/placement.rs

crates/ruff_python_formatter/src/expression/mod.rs

MichaReiser · 2023-10-16T03:01:56Z

crates/ruff_python_formatter/src/expression/mod.rs

+        Expr::BoolOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::NamedExpr(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::BinOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::UnaryOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Lambda(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::IfExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Dict(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Set(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::ListComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::SetComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::DictComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::GeneratorExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Await(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Yield(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::YieldFrom(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Compare(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Call(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::FormattedValue(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::FString(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Constant(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Attribute(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Subscript(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Starred(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Name(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::List(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Tuple(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::Slice(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),
+        Expr::IpyEscapeCommand(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f),


Nit: The main benefit of FormatRuleWith is that it wraps the object to format with how it gets formatted. I would probably lean towards calling the relevant format rules directly (fewer abstractions, easier to understand)

Expr::Call => FormatExprCall::default().fmt_fields(expr, f)

I prefer this one for avoiding repeating the -> Format association.

That's fair and up to you. It just requires a few more brain cycles to resolve FormatNodeRule::fmt_fields(expr.format().rule(), expr, f) to FormatExprFString::default().fmt_fields(expr, f)

konstin · 2023-10-17T10:50:10Z

crates/ruff_python_formatter/src/expression/mod.rs

+) -> FormatResult<()> {
+    // First part: Split the comments
+
+    // TODO(konstin): This is copied from `parenthesized_range`, except that we don't have the


I'm still not sure what to do about this, otherwise this PR is ready

I don't mind the duplication. We could implement a helper that returns an Iterator over the parentheses (opening and closing) and reuse it in parenthesized_range and here.

MichaReiser · 2023-10-18T23:24:14Z

Could you update the test plan with the similarity index table to see if there are any changes.

crates/ruff_python_formatter/src/comments/placement.rs

MichaReiser · 2023-10-18T23:30:05Z

crates/ruff_python_formatter/src/expression/mod.rs

+) -> FormatResult<()> {
+    // First part: Split the comments
+
+    // TODO(konstin): This is copied from `parenthesized_range`, except that we don't have the


I don't mind the duplication. We could implement a helper that returns an Iterator over the parentheses (opening and closing) and reuse it in parenthesized_range and here.

MichaReiser · 2023-10-18T23:33:28Z

crates/ruff_python_formatter/src/expression/mod.rs

+) -> FormatResult<()> {
+    // First part: Split the comments
+
+    // TODO(konstin): This is copied from `parenthesized_range`, except that we don't have the


parenthesized_range has a special case for call expressions like call(a) to avoid counting the ( and ) as the parentheses of this node. Is it safe to not special-tread call expressions here and if so why?

( call # sneaky ( # comments a ) # more )

The problem is that the expression would be a but a doesn't know its parent

crates/ruff_python_formatter/src/expression/mod.rs

MichaReiser · 2023-10-18T23:38:04Z

crates/ruff_python_formatter/src/expression/mod.rs

+    // * the closing parenthesis
+    // * outer trailing comments
+
+    let fmt_fields = format_with(|f| match expression {


Nit: It could make sense to extract this into it's own struct FormatExprFields to not make this function longer than absolutely necessary (It kind of breaks the reading flow, there's the comment above explaining what is happening, but it is then followed by this somewhat unrelated boilerplate)

MichaReiser · 2023-10-18T23:39:22Z

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py

-with (a # trailing same line comment
-    # trailing own line comment
-    ) as b: ...
-


What's the reason for removing this test case?

It's a duplicate, sorry should have added a PR comment

Co-authored-by: Micha Reiser <micha@reiser.io>

konstin · 2023-10-19T09:18:07Z

Added the similarity index table to the description, this had more impact than i had expected

github-actions · 2023-10-19T09:30:41Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

konstin mentioned this pull request Oct 10, 2023

[docs] Default to following the system dark/light mode #7888

Merged

konstin force-pushed the Comments-outside-expression-parentheses branch from 2a1fa16 to 55b2da1 Compare October 10, 2023 14:28

konstin added the formatter Related to the formatter label Oct 10, 2023

konstin requested a review from MichaReiser October 11, 2023 14:02

MichaReiser reviewed Oct 16, 2023

View reviewed changes

konstin force-pushed the Comments-outside-expression-parentheses branch from 9addbb3 to 504c7af Compare October 17, 2023 10:16

konstin marked this pull request as ready for review October 17, 2023 10:49

konstin commented Oct 17, 2023

View reviewed changes

konstin requested a review from MichaReiser October 18, 2023 19:34

MichaReiser approved these changes Oct 18, 2023

View reviewed changes

konstin force-pushed the Comments-outside-expression-parentheses branch from a40819c to d61caea Compare October 19, 2023 09:08

konstin enabled auto-merge (squash) October 19, 2023 09:08

konstin and others added 10 commits October 19, 2023 11:14

Comments outside expression parentheses

c0d7980

Code style

38d4683

Document missing parent problem

53603bb

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

4fc4e43

Co-authored-by: Micha Reiser <micha@reiser.io>

Review

c225151

clippy

b858725

comment

6fd44ee

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

3a45c14

Co-authored-by: Micha Reiser <micha@reiser.io>

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

3dea426

Co-authored-by: Micha Reiser <micha@reiser.io>

Review: Introduce parentheses_iterator

e4ef5b0

konstin force-pushed the Comments-outside-expression-parentheses branch from d61caea to e4ef5b0 Compare October 19, 2023 09:15

konstin merged commit 8f9753f into main Oct 19, 2023
15 checks passed

konstin deleted the Comments-outside-expression-parentheses branch October 19, 2023 09:24

p-linnane mentioned this pull request Oct 19, 2023

ruff 0.1.1 Homebrew/homebrew-core#151871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments outside expression parentheses #7873

Comments outside expression parentheses #7873

konstin commented Oct 9, 2023 •

edited

Loading

konstin commented Oct 12, 2023

MichaReiser left a comment

MichaReiser Oct 16, 2023 •

edited

Loading

konstin Oct 17, 2023

MichaReiser Oct 18, 2023

konstin Oct 17, 2023

MichaReiser Oct 18, 2023

MichaReiser commented Oct 18, 2023

MichaReiser Oct 18, 2023

MichaReiser Oct 18, 2023

konstin Oct 19, 2023

MichaReiser Oct 18, 2023

MichaReiser Oct 18, 2023

konstin Oct 19, 2023

konstin commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 19, 2023

Comments outside expression parentheses #7873

Comments outside expression parentheses #7873

Conversation

konstin commented Oct 9, 2023 • edited Loading

Summary

Test Plan

konstin commented Oct 12, 2023

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser Oct 16, 2023 • edited Loading

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

MichaReiser commented Oct 18, 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

konstin commented Oct 19, 2023 • edited Loading

github-actions bot commented Oct 19, 2023

PR Check Results

Ecosystem

konstin commented Oct 9, 2023 •

edited

Loading

MichaReiser Oct 16, 2023 •

edited

Loading

konstin commented Oct 19, 2023 •

edited

Loading