Skip to content

Avoid hard line break after dangling open-parenthesis comments#6380

Merged
charliermarsh merged 1 commit intomainfrom
charlie/trailing-comment
Aug 7, 2023
Merged

Avoid hard line break after dangling open-parenthesis comments#6380
charliermarsh merged 1 commit intomainfrom
charlie/trailing-comment

Conversation

@charliermarsh
Copy link
Member

Summary

Given:

[  # comment
    first,
    second,
    third
]  # another comment

We were adding a hard line break as part of the formatting of # comment, which led to the following formatting:

[first, second, third]  # comment
  # another comment

Closes #6367.


Ok(())
}
}
Copy link
Member Author

@charliermarsh charliermarsh Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A dedicated formatter for these kinds of open-parenthetical methods, rather than using the generic dangling_comments formatter which always hard-breaks after comments.

@charliermarsh charliermarsh force-pushed the charlie/trailing-comment branch from 44a1b29 to 125504c Compare August 7, 2023 02:19
@charliermarsh charliermarsh changed the title Avoid hard line break after dangling inner-parenthesis comments Avoid hard line break after dangling open-parenthesis comments Aug 7, 2023
@charliermarsh charliermarsh requested a review from konstin August 7, 2023 02:46
@charliermarsh charliermarsh force-pushed the charlie/trailing-comment branch from 125504c to 5fef49b Compare August 7, 2023 02:48
@charliermarsh charliermarsh changed the base branch from main to charlie/arguments-dangling August 7, 2023 02:48
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.3±0.52ms     4.4 MB/sec    1.02      9.5±0.58ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.00  1779.9±95.39µs     9.4 MB/sec    1.05  1874.2±107.38µs     8.9 MB/sec
formatter/numpy/globals.py                 1.01   229.4±25.79µs    12.9 MB/sec    1.00   227.0±15.22µs    13.0 MB/sec
formatter/pydantic/types.py                1.00      3.9±0.25ms     6.6 MB/sec    1.03      4.0±0.26ms     6.4 MB/sec
linter/all-rules/large/dataset.py          1.04     13.0±0.79ms     3.1 MB/sec    1.00     12.5±0.75ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.02      3.3±0.29ms     5.0 MB/sec    1.00      3.3±0.15ms     5.1 MB/sec
linter/all-rules/numpy/globals.py          1.00   482.9±33.07µs     6.1 MB/sec    1.01   486.7±32.76µs     6.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.9±0.32ms     4.3 MB/sec    1.06      6.3±0.34ms     4.0 MB/sec
linter/default-rules/large/dataset.py      1.00      6.6±0.31ms     6.2 MB/sec    1.04      6.9±0.32ms     5.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1441.5±80.11µs    11.6 MB/sec    1.01  1449.6±81.72µs    11.5 MB/sec
linter/default-rules/numpy/globals.py      1.04   169.9±12.15µs    17.4 MB/sec    1.00   163.5±11.27µs    18.0 MB/sec
linter/default-rules/pydantic/types.py     1.01      2.9±0.14ms     8.8 MB/sec    1.00      2.9±0.16ms     8.9 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.9±0.05ms     5.1 MB/sec    1.00      7.9±0.06ms     5.1 MB/sec
formatter/numpy/ctypeslib.py               1.00   1518.7±9.23µs    11.0 MB/sec    1.00  1519.8±31.79µs    11.0 MB/sec
formatter/numpy/globals.py                 1.00    153.3±1.63µs    19.3 MB/sec    1.01    154.8±4.75µs    19.1 MB/sec
formatter/pydantic/types.py                1.00      3.3±0.03ms     7.6 MB/sec    1.00      3.3±0.03ms     7.7 MB/sec
linter/all-rules/large/dataset.py          1.00     10.3±0.10ms     4.0 MB/sec    1.00     10.3±0.07ms     4.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.7±0.01ms     6.1 MB/sec    1.00      2.7±0.02ms     6.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    294.8±2.24µs    10.0 MB/sec    1.01    296.5±3.25µs    10.0 MB/sec
linter/all-rules/pydantic/types.py         1.00      4.6±0.04ms     5.5 MB/sec    1.00      4.6±0.03ms     5.5 MB/sec
linter/default-rules/large/dataset.py      1.00      5.6±0.04ms     7.2 MB/sec    1.00      5.6±0.05ms     7.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1104.3±10.16µs    15.1 MB/sec    1.00   1105.6±9.20µs    15.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    115.1±1.19µs    25.6 MB/sec    1.00    115.5±1.13µs    25.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.4±0.03ms    10.5 MB/sec    1.01      2.4±0.02ms    10.4 MB/sec

@charliermarsh charliermarsh force-pushed the charlie/trailing-comment branch from 5fef49b to 7332d43 Compare August 7, 2023 03:43
@@ -155,7 +155,7 @@ impl<'ast> Format<PyFormatContext<'ast>> for FormatParenthesized<'_, 'ast> {
} else {
group(&format_args![
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this outer group anymore? There's nothing anymore that expands the parent (dangling comments always rendered a hard line break, forcing the group to expand). Or should parenthetical_comments render an expand_parent?

)
- )
-)
+a = int(int(int(6))) # type: ignore # type: ignore # type: ignore # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO: The old formatting expressed the intent of the user better.

We, so far, have taken the stand that trailing comments should remain as closely as possible to what they originally commented (see internal notion). Meaning they should, by default, expand their parent. I think this should also help that ignore comments move to far away, changing what they suppress.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think this should be investigated separately from this PR.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to do what's described but it is a deviation from Black:

--- a/crates/ruff_python_formatter/src/comments/format.rs
+++ b/crates/ruff_python_formatter/src/comments/format.rs
@@ -259,11 +259,10 @@ impl Format<PyFormatContext<'_>> for FormatDanglingOpenParenthesisComments<'_> {

             write!(
                 f,
-                [line_suffix(&format_args![
-                    space(),
-                    space(),
-                    format_comment(comment)
-                ])]
+                [
+                    line_suffix(&format_args![space(), space(), format_comment(comment)]),
+                    expand_parent()
+                ]
             )?;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will put up a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One feature that Ruff's architecture enables is that we can move comments. This is much harder in Black. I think we should use it and be opinionated about where comments should be placed and how they get formatted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up here: #6389

@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 7, 2023
Base automatically changed from charlie/arguments-dangling to main August 7, 2023 13:43
@charliermarsh charliermarsh force-pushed the charlie/trailing-comment branch from 7332d43 to b312f42 Compare August 7, 2023 13:51
@charliermarsh charliermarsh requested a review from konstin August 7, 2023 13:51
@charliermarsh charliermarsh enabled auto-merge (squash) August 7, 2023 13:55
@charliermarsh charliermarsh disabled auto-merge August 7, 2023 13:58
@charliermarsh charliermarsh force-pushed the charlie/trailing-comment branch from b312f42 to 9b2557c Compare August 7, 2023 14:05
@charliermarsh charliermarsh enabled auto-merge (squash) August 7, 2023 14:05
@charliermarsh charliermarsh merged commit b763973 into main Aug 7, 2023
@charliermarsh charliermarsh deleted the charlie/trailing-comment branch August 7, 2023 14:15
durumu pushed a commit to durumu/ruff that referenced this pull request Aug 12, 2023
…l-sh#6380)

## Summary

Given:

```python
[  # comment
    first,
    second,
    third
]  # another comment
```

We were adding a hard line break as part of the formatting of `#
comment`, which led to the following formatting:

```python
[first, second, third]  # comment
  # another comment
```

Closes astral-sh#6367.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

formatter Related to the formatter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Formatter: Trailing comment on list gets formatted to a separate line

3 participants