Skip to content

Handle right parens in join comma builder#5711

Merged
MichaReiser merged 1 commit intomainfrom
right-parens-join-comma
Jul 12, 2023
Merged

Handle right parens in join comma builder#5711
MichaReiser merged 1 commit intomainfrom
right-parens-join-comma

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jul 12, 2023

Summary

The JoinCommaSeparatedBuilder only tested if the first token after the node is a ,.
However, this always fails for expressions with parentheses, because the parentheses are not part of the expression's range.

This PR changes the JoinCommaSeparatedBuilder to skip right parens. However, we need to be careful, because nested(call(arg),) only the outer call has a trailing comma
in this example, not the inner arg. The way this is implemented is by also passing an approxiamted end of the sequence (normally the end of the enclosing node, but we don't always know precisely), and only search between the end of the last node and the end of the sequence.

I further went along and also fixed an issue where single argument call expressions were always parenthesized because is_expression_parenthesized doesn't know that the parens from the arguments belong to the call. The way to solve this is by handling parentheses
manually inside of FormatCallExpr if the call only has a single argument.

Test Plan

See updated snapshot tests.

@MichaReiser MichaReiser requested a review from konstin July 12, 2023 13:38
@MichaReiser MichaReiser added the formatter Related to the formatter label Jul 12, 2023
@MichaReiser
Copy link
Member Author

MichaReiser commented Jul 12, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser linked an issue Jul 12, 2023 that may be closed by this pull request
Single argument are always parenthesized in call expressions #5707
Closed
MichaReiser
MichaReiser commented Jul 12, 2023
View reviewed changes
...matter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap

func_with_bad_parens_that_wont_fit_in_one_line(
"short string that should have parens stripped", x, y, z
("short string that should have parens stripped"), x, y, z
Copy link
Member Author

@MichaReiser MichaReiser Jul 12, 2023

Choose a reason for hiding this comment

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

I tested this locally, black preserves the parentheses, regardless of what the text says

Copy link
Member

@konstin konstin Jul 12, 2023

Choose a reason for hiding this comment

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

you can open a black bug :P

@MichaReiser MichaReiser mentioned this pull request Jul 12, 2023
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2023 •
edited
Loading

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      9.4±0.04ms     4.3 MB/sec    1.00      9.4±0.12ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.01      2.2±0.01ms     7.5 MB/sec    1.00      2.2±0.01ms     7.6 MB/sec
formatter/numpy/globals.py                 1.00    250.0±6.19µs    11.8 MB/sec    1.04   260.3±15.89µs    11.3 MB/sec
formatter/pydantic/types.py                1.01      4.7±0.08ms     5.4 MB/sec    1.00      4.7±0.06ms     5.4 MB/sec
linter/all-rules/large/dataset.py          1.04     17.1±0.53ms     2.4 MB/sec    1.00     16.4±0.53ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      4.3±0.14ms     3.9 MB/sec    1.00      4.2±0.15ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    518.6±1.31µs     5.7 MB/sec    1.00    518.1±2.03µs     5.7 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.2±0.04ms     3.5 MB/sec    1.00      7.2±0.08ms     3.5 MB/sec
linter/default-rules/large/dataset.py      1.01      8.1±0.04ms     5.1 MB/sec    1.00      8.0±0.12ms     5.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1754.2±11.10µs     9.5 MB/sec    1.01  1769.4±11.97µs     9.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    202.8±3.91µs    14.6 MB/sec    1.00    202.9±0.87µs    14.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.05ms     7.0 MB/sec    1.01      3.7±0.02ms     7.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.4±0.13ms     4.3 MB/sec    1.00      9.5±0.10ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.1±0.02ms     7.9 MB/sec    1.01      2.1±0.03ms     7.8 MB/sec
formatter/numpy/globals.py                 1.00    226.4±3.07µs    13.0 MB/sec    1.01    229.3±6.39µs    12.9 MB/sec
formatter/pydantic/types.py                1.00      4.7±0.03ms     5.5 MB/sec    1.01      4.7±0.03ms     5.4 MB/sec
linter/all-rules/large/dataset.py          1.00     15.6±0.16ms     2.6 MB/sec    1.00     15.7±0.07ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.1±0.04ms     4.0 MB/sec    1.00      4.1±0.02ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    423.1±5.51µs     7.0 MB/sec    1.00    424.6±5.50µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.0±0.05ms     3.6 MB/sec    1.01      7.1±0.04ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.00      8.0±0.02ms     5.1 MB/sec    1.02      8.2±0.06ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1638.2±11.37µs    10.2 MB/sec    1.02  1668.2±10.45µs    10.0 MB/sec
linter/default-rules/numpy/globals.py      1.00    177.8±1.65µs    16.6 MB/sec    1.01    179.5±1.51µs    16.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.02ms     7.1 MB/sec    1.01      3.6±0.01ms     7.0 MB/sec

konstin
konstin approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

good catch

...matter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap

func_with_bad_parens_that_wont_fit_in_one_line(
"short string that should have parens stripped", x, y, z
("short string that should have parens stripped"), x, y, z
Copy link
Member

@konstin konstin Jul 12, 2023

Choose a reason for hiding this comment

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

you can open a black bug :P

@MichaReiser MichaReiser merged commit 653429b into main Jul 12, 2023
@MichaReiser MichaReiser deleted the right-parens-join-comma branch July 12, 2023 16:21
konstin pushed a commit that referenced this pull request Jul 19, 2023
@MichaReiser @konstin
Handle right parens in join comma builder (#5711)
f974e77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@konstin konstin konstin approved these changes

Assignees

No one assigned

Labels

formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Single argument are always parenthesized in call expressions

2 participants

@MichaReiser @konstin