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

Handle right parens in join comma builder #5711

Merged
merged 1 commit into from Jul 12, 2023
Merged

Conversation

MichaReiser
Copy link
Member

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

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
)

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

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

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2023

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

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

)

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

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
16 checks passed
@MichaReiser MichaReiser deleted the right-parens-join-comma branch July 12, 2023 16:21
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.

Single argument are always parenthesized in call expressions
2 participants