Fixup comment handling on opening parenthesis in function definition by charliermarsh · Pull Request #6381 · astral-sh/ruff

charliermarsh · 2023-08-07T02:17:36Z

Summary

I noticed some deviations in how we treat dangling comments that hug the opening parenthesis for function definitions.

For example, given:

def f(  # first
    # second
):  # third
    ...

We currently format as:

def f(
      # first
    # second
):  # third
    ...

This PR adds the proper opening-parenthesis dangling comment handling for function parameters. Specifically, as with all other parenthesized nodes, we now detect that dangling comment in placement.rs and handle it in parameters.rs. We have to take some care in that file, since we have multiple "kinds" of dangling comments, but I added a bunch of test cases that we now format identically to Black.

Test Plan

cargo test

Before:

zulip: 0.99388
django: 0.99784
warehouse: 0.99504
transformers: 0.99404
cpython: 0.75913
typeshed: 0.74364

After:

zulip: 0.99386
django: 0.99784
warehouse: 0.99504
transformers: 0.99404
cpython: 0.75913
typeshed: 0.74409

Meaningful improvement on typeshed, minor decrease on zulip.

charliermarsh · 2023-08-07T02:21:27Z

crates/ruff_python_formatter/src/other/parameters.rs

-            write!(f, [parenthesized("(", &group(&format_inner), ")")])
+            write!(
+                f,
+                [parenthesized_with_dangling_comments(


Now using the same utilities as all the other nodes (parenthesized_with_dangling_comments, empty_parenthesized_with_dangling_comments).

github-actions · 2023-08-07T03:24:09Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.05     10.1±0.14ms     4.0 MB/sec    1.00      9.6±0.10ms     4.2 MB/sec
formatter/numpy/ctypeslib.py               1.02  1936.5±39.72µs     8.6 MB/sec    1.00   1905.4±8.72µs     8.7 MB/sec
formatter/numpy/globals.py                 1.03    218.2±4.03µs    13.5 MB/sec    1.00    212.3±2.92µs    13.9 MB/sec
formatter/pydantic/types.py                1.03      4.2±0.09ms     6.1 MB/sec    1.00      4.1±0.03ms     6.3 MB/sec
linter/all-rules/large/dataset.py          1.00     12.1±0.09ms     3.4 MB/sec    1.01     12.2±0.14ms     3.3 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.2±0.06ms     5.3 MB/sec    1.03      3.2±0.02ms     5.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    446.5±5.44µs     6.6 MB/sec    1.01    450.6±5.71µs     6.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.5±0.05ms     4.6 MB/sec    1.00      5.5±0.09ms     4.6 MB/sec
linter/default-rules/large/dataset.py      1.00      6.3±0.06ms     6.5 MB/sec    1.02      6.4±0.05ms     6.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1345.4±10.46µs    12.4 MB/sec    1.01   1359.2±9.91µs    12.3 MB/sec
linter/default-rules/numpy/globals.py      1.00    151.6±2.59µs    19.5 MB/sec    1.01    153.8±2.69µs    19.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.8±0.02ms     9.1 MB/sec    1.01      2.8±0.03ms     9.1 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     10.3±0.17ms     4.0 MB/sec    1.00     10.3±0.22ms     4.0 MB/sec
formatter/numpy/ctypeslib.py               1.00  1932.0±28.55µs     8.6 MB/sec    1.00  1923.8±45.98µs     8.7 MB/sec
formatter/numpy/globals.py                 1.01    215.3±6.71µs    13.7 MB/sec    1.00    214.2±8.63µs    13.8 MB/sec
formatter/pydantic/types.py                1.00      4.3±0.15ms     5.9 MB/sec    1.00      4.3±0.08ms     5.9 MB/sec
linter/all-rules/large/dataset.py          1.01     13.4±0.27ms     3.0 MB/sec    1.00     13.3±0.32ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.6±0.08ms     4.6 MB/sec    1.00      3.6±0.10ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.03    440.1±7.47µs     6.7 MB/sec    1.00    427.7±6.90µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.1±0.15ms     4.2 MB/sec    1.00      6.0±0.15ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.01      7.1±0.12ms     5.7 MB/sec    1.00      7.0±0.16ms     5.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1440.2±22.81µs    11.6 MB/sec    1.00  1438.4±24.41µs    11.6 MB/sec
linter/default-rules/numpy/globals.py      1.02    166.7±3.81µs    17.7 MB/sec    1.00    163.6±2.82µs    18.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.06ms     8.2 MB/sec    1.02      3.1±0.05ms     8.1 MB/sec

MichaReiser · 2023-08-07T07:41:19Z

crates/ruff_python_formatter/src/other/parameters.rs

+                .skip_trivia()
+                .skip_while(|t| {
+                    matches!(
+                        t.kind(),
+                        SimpleTokenKind::LParen
+                            | SimpleTokenKind::LBrace
+                            | SimpleTokenKind::LBracket
+                    )
+                });


Unrelated to this PR but just popped to my head (may be relevant for other places where we apply the same logic). What happens if we have:

call(( a # test ))

I would expect this to be a comment of a and not a dangling arguments comment.

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

crates/ruff_python_formatter/src/other/parameters.rs

MichaReiser · 2023-08-07T07:54:30Z

Please update your test plan with the similarity index.

crates/ruff_python_formatter/src/other/parameters.rs

konstin · 2023-08-07T11:18:55Z

This fixes an instability with zulip, can you add

x = () - a(#
b)

to the tests?

charliermarsh · 2023-08-07T17:35:36Z

@konstin - That's a call though, this only touches function definitions, is it related?

konstin · 2023-08-07T20:03:45Z

sorry, i got confused

…stral-sh#6381) ## Summary I noticed some deviations in how we treat dangling comments that hug the opening parenthesis for function definitions. For example, given: ```python def f( # first # second ): # third ... ``` We currently format as: ```python def f( # first # second ): # third ... ``` This PR adds the proper opening-parenthesis dangling comment handling for function parameters. Specifically, as with all other parenthesized nodes, we now detect that dangling comment in `placement.rs` and handle it in `parameters.rs`. We have to take some care in that file, since we have multiple "kinds" of dangling comments, but I added a bunch of test cases that we now format identically to Black. ## Test Plan `cargo test` Before: - `zulip`: 0.99388 - `django`: 0.99784 - `warehouse`: 0.99504 - `transformers`: 0.99404 - `cpython`: 0.75913 - `typeshed`: 0.74364 After: - `zulip`: 0.99386 - `django`: 0.99784 - `warehouse`: 0.99504 - `transformers`: 0.99404 - `cpython`: 0.75913 - `typeshed`: 0.74409 Meaningful improvement on `typeshed`, minor decrease on `zulip`.

charliermarsh requested a review from konstin August 7, 2023 02:17

charliermarsh added the formatter Related to the formatter label Aug 7, 2023

charliermarsh force-pushed the charlie/trailing-comment branch from 44a1b29 to 125504c Compare August 7, 2023 02:19

charliermarsh force-pushed the charlie/inner-parenthesis-function branch from b63851f to efff187 Compare August 7, 2023 02:21

charliermarsh commented Aug 7, 2023

View reviewed changes

charliermarsh force-pushed the charlie/inner-parenthesis-function branch from efff187 to 8237b5d Compare August 7, 2023 02:21

charliermarsh force-pushed the charlie/trailing-comment branch from 125504c to 5fef49b Compare August 7, 2023 02:48

charliermarsh force-pushed the charlie/inner-parenthesis-function branch from 8237b5d to 0de408d Compare August 7, 2023 02:48

charliermarsh force-pushed the charlie/trailing-comment branch from 5fef49b to 7332d43 Compare August 7, 2023 03:43

charliermarsh force-pushed the charlie/inner-parenthesis-function branch from 0de408d to 78129e2 Compare August 7, 2023 03:44

MichaReiser reviewed Aug 7, 2023

View reviewed changes

konstin approved these changes Aug 7, 2023

View reviewed changes

crates/ruff_python_formatter/src/other/parameters.rs Outdated Show resolved Hide resolved

crates/ruff_python_formatter/src/other/parameters.rs Show resolved Hide resolved

konstin mentioned this pull request Aug 7, 2023

Add formatting for StmtMatch #6286

Merged

konstin force-pushed the charlie/inner-parenthesis-function branch from 2873e6e to 78129e2 Compare August 7, 2023 11:23

charliermarsh force-pushed the charlie/trailing-comment branch 2 times, most recently from b312f42 to 9b2557c Compare August 7, 2023 14:05

Base automatically changed from charlie/trailing-comment to main August 7, 2023 14:15

charliermarsh force-pushed the charlie/inner-parenthesis-function branch 2 times, most recently from d55a7cf to 26b1266 Compare August 7, 2023 17:31

Fixup comment handling on inner parenthesis in function definition

3eed713

charliermarsh force-pushed the charlie/inner-parenthesis-function branch from 26b1266 to 3eed713 Compare August 7, 2023 17:32

charliermarsh merged commit a637b8b into main Aug 7, 2023

charliermarsh deleted the charlie/inner-parenthesis-function branch August 7, 2023 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup comment handling on opening parenthesis in function definition#6381

Fixup comment handling on opening parenthesis in function definition#6381
charliermarsh merged 1 commit intomainfrom
charlie/inner-parenthesis-function

charliermarsh commented Aug 7, 2023 •

edited

Loading

Uh oh!

charliermarsh Aug 7, 2023

Uh oh!

github-actions bot commented Aug 7, 2023 •

edited

Loading

Uh oh!

MichaReiser Aug 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Aug 7, 2023

Uh oh!

Uh oh!

Uh oh!

konstin commented Aug 7, 2023

Uh oh!

charliermarsh commented Aug 7, 2023

Uh oh!

konstin commented Aug 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

charliermarsh commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

charliermarsh Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Benchmark

Linux

Windows

Uh oh!

MichaReiser Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Aug 7, 2023

Uh oh!

Uh oh!

Uh oh!

konstin commented Aug 7, 2023

Uh oh!

charliermarsh commented Aug 7, 2023

Uh oh!

konstin commented Aug 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

charliermarsh commented Aug 7, 2023 •

edited

Loading

github-actions bot commented Aug 7, 2023 •

edited

Loading