Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 7, 2023

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 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 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

crates/ruff_python_formatter/src/comments/format.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/expression/parentheses.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/comments/format.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/comments/format.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/comments/format.rs Outdated Show resolved Hide resolved
@@ -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 enabled auto-merge (squash) August 7, 2023 13:55
@charliermarsh charliermarsh enabled auto-merge (squash) August 7, 2023 14:05
@charliermarsh charliermarsh merged commit b763973 into main Aug 7, 2023
16 checks passed
@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