Fixup comment handling on opening parenthesis in function definition #6381
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
charliermarsh
force-pushed
the
charlie/trailing-comment
branch
from
44a1b29
to
125504c
Compare
charliermarsh
force-pushed
the
charlie/inner-parenthesis-function
branch
from
b63851f
to
efff187
Compare
charliermarsh
commented
Aug 7, 2023
| write!(f, [parenthesized("(", &group(&format_inner), ")")]) | ||
| write!( | ||
| f, | ||
| [parenthesized_with_dangling_comments( |
There was a problem hiding this comment.
Now using the same utilities as all the other nodes (parenthesized_with_dangling_comments, empty_parenthesized_with_dangling_comments).
charliermarsh
force-pushed
the
charlie/inner-parenthesis-function
branch
from
efff187
to
8237b5d
Compare
charliermarsh
force-pushed
the
charlie/trailing-comment
branch
from
125504c
to
5fef49b
Compare
charliermarsh
force-pushed
the
charlie/inner-parenthesis-function
branch
from
8237b5d
to
0de408d
Compare
PR Check ResultsBenchmarkLinuxWindows |
charliermarsh
force-pushed
the
charlie/trailing-comment
branch
from
5fef49b
to
7332d43
Compare
charliermarsh
force-pushed
the
charlie/inner-parenthesis-function
branch
from
0de408d
to
78129e2
Compare
MichaReiser
reviewed
Aug 7, 2023
Comment on lines
+84
to
+92
| .skip_trivia() | ||
| .skip_while(|t| { | ||
| matches!( | ||
| t.kind(), | ||
| SimpleTokenKind::LParen | ||
| | SimpleTokenKind::LBrace | ||
| | SimpleTokenKind::LBracket | ||
| ) | ||
| }); |
There was a problem hiding this comment.
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
Show resolved
Hide resolved
|
Please update your test plan with the similarity index. |
konstin
approved these changes
Aug 7, 2023
|
This fixes an instability with zulip, can you add x = () - a(#
b)to the tests? |
konstin
force-pushed
the
charlie/inner-parenthesis-function
branch
from
2873e6e
to
78129e2
Compare
charliermarsh
force-pushed
the
charlie/trailing-comment
branch
2 times, most recently
from
b312f42
to
9b2557c
Compare
charliermarsh
force-pushed
the
charlie/inner-parenthesis-function
branch
2 times, most recently
from
d55a7cf
to
26b1266
Compare
charliermarsh
force-pushed
the
charlie/inner-parenthesis-function
branch
from
26b1266
to
3eed713
Compare
|
@konstin - That's a call though, this only touches function definitions, is it related? |
|
sorry, i got confused |
durumu
pushed a commit
to durumu/ruff
that referenced
this pull request
Aug 12, 2023
…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`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
I noticed some deviations in how we treat dangling comments that hug the opening parenthesis for function definitions.
For example, given:
We currently format as:
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.rsand handle it inparameters.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 testBefore:
zulip: 0.99388django: 0.99784warehouse: 0.99504transformers: 0.99404cpython: 0.75913typeshed: 0.74364After:
zulip: 0.99386django: 0.99784warehouse: 0.99504transformers: 0.99404cpython: 0.75913typeshed: 0.74409Meaningful improvement on
typeshed, minor decrease onzulip.