Skip to content

Update TupleParentheses usage#5810

Merged
MichaReiser merged 12 commits intoastral-sh:mainfrom
cnpryer:chore-tuple-paren
Jul 24, 2023
Merged

Update TupleParentheses usage#5810
MichaReiser merged 12 commits intoastral-sh:mainfrom
cnpryer:chore-tuple-paren

Conversation

@cnpryer
Copy link
Contributor

@cnpryer cnpryer commented Jul 16, 2023 •
edited
Loading

Summary

Implements a potentially more re-usable naming convention for TupleParentheses.

Subscript case -> Preserve - "Preserve the tuple's parentheses if they are there"
SkipInsideLambda case -> Never - "Parentheses are never needed with this parent" (#5806; comment)
Comprehension case -> Never - "Parentheses are never needed with this parent" (#5790; comment)
StripInsideForLoop case -> NeverPreserve - "Parentheses are never preserved in special cases" (I interpret "Preserve" as "not always necessary but keep them anyway")

The counter to this I'd think is that there may be benefit to having node-specific differences.

Test Plan

Existing snapshots.

Questions

  • Where would TupleParentheses::Expr(...) be helpful in some of these cases (if at all)? (answered)

Related:

@cnpryer cnpryer force-pushed the chore-tuple-paren branch 2 times, most recently from 3342a65 to 0d34ada Compare July 16, 2023 17:10
@cnpryer cnpryer mentioned this pull request Jul 16, 2023
Merged
2 tasks
@github-actions
Copy link
Contributor

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

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      9.5±0.03ms     4.3 MB/sec    1.00      9.4±0.05ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.00   1879.5±2.54µs     8.9 MB/sec    1.00   1877.0±3.25µs     8.9 MB/sec
formatter/numpy/globals.py                 1.00    205.2±0.56µs    14.4 MB/sec    1.02    209.9±0.28µs    14.1 MB/sec
formatter/pydantic/types.py                1.00      4.1±0.01ms     6.3 MB/sec    1.00      4.1±0.01ms     6.3 MB/sec
linter/all-rules/large/dataset.py          1.01     13.1±0.05ms     3.1 MB/sec    1.00     13.0±0.05ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.3±0.01ms     5.1 MB/sec    1.00      3.3±0.01ms     5.1 MB/sec
linter/all-rules/numpy/globals.py          1.01    435.3±4.34µs     6.8 MB/sec    1.00    433.0±0.85µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.02      6.0±0.04ms     4.3 MB/sec    1.00      5.9±0.03ms     4.3 MB/sec
linter/default-rules/large/dataset.py      1.01      6.6±0.02ms     6.2 MB/sec    1.00      6.5±0.02ms     6.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1406.8±2.11µs    11.8 MB/sec    1.00   1400.6±2.28µs    11.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    156.0±1.16µs    18.9 MB/sec    1.00    156.2±1.00µs    18.9 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.0±0.01ms     8.5 MB/sec    1.00      3.0±0.01ms     8.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     12.4±0.50ms     3.3 MB/sec    1.15     14.2±0.40ms     2.9 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.5±0.10ms     6.8 MB/sec    1.13      2.8±0.08ms     6.0 MB/sec
formatter/numpy/globals.py                 1.00   284.9±14.34µs    10.4 MB/sec    1.10   313.2±12.10µs     9.4 MB/sec
formatter/pydantic/types.py                1.00      5.3±0.20ms     4.8 MB/sec    1.14      6.0±0.17ms     4.2 MB/sec
linter/all-rules/large/dataset.py          1.00     18.0±0.61ms     2.3 MB/sec    1.05     18.9±0.95ms     2.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      4.9±0.28ms     3.4 MB/sec    1.00      4.8±0.19ms     3.5 MB/sec
linter/all-rules/numpy/globals.py          1.06   610.8±29.39µs     4.8 MB/sec    1.00   575.3±30.83µs     5.1 MB/sec
linter/all-rules/pydantic/types.py         1.02      8.2±0.29ms     3.1 MB/sec    1.00      8.0±0.34ms     3.2 MB/sec
linter/default-rules/large/dataset.py      1.00      9.4±0.42ms     4.3 MB/sec    1.08     10.2±0.58ms     4.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1958.9±86.55µs     8.5 MB/sec    1.04      2.0±0.06ms     8.2 MB/sec
linter/default-rules/numpy/globals.py      1.00   246.5±19.08µs    12.0 MB/sec    1.01   249.5±11.91µs    11.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.4±0.24ms     5.8 MB/sec    1.00      4.4±0.15ms     5.8 MB/sec

@cnpryer cnpryer marked this pull request as draft July 16, 2023 17:24
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from 0d34ada to d33384f Compare July 16, 2023 17:42
@cnpryer cnpryer marked this pull request as ready for review July 16, 2023 17:44
@cnpryer cnpryer force-pushed the chore-tuple-paren branch 2 times, most recently from 7b36639 to c3e4d6b Compare July 16, 2023 17:52
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from c3e4d6b to 45f9d84 Compare July 16, 2023 17:52
@cnpryer cnpryer mentioned this pull request Jul 16, 2023
Merged
5 tasks
@konstin konstin added the formatter Related to the formatter label Jul 16, 2023
cnpryer
cnpryer commented Jul 16, 2023
View reviewed changes
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from 7a323ed to ec9b23d Compare July 16, 2023 18:46
@MichaReiser MichaReiser requested a review from konstin July 17, 2023 07:45
konstin
konstin reviewed Jul 18, 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.

Thanks for tackling something as finicky as the tuple parentheses! We discussed this in team and we realized naming these is hard with all the black edge cases. It would be great if we could reduce this enum to as few variants as possible to reduce complexity

@cnpryer
Copy link
Contributor Author

cnpryer commented Jul 19, 2023

Really appreciate all of this direction. I'm going to wrap up #5806 and then #5790. Then I can come back here with everything you need to see if this is even something that's worth merging.

@cnpryer cnpryer force-pushed the chore-tuple-paren branch from b0ecc10 to 89268f0 Compare July 19, 2023 01:58
@cnpryer
Copy link
Contributor Author

cnpryer commented Jul 20, 2023 •
edited
Loading

I'm not in love with TupleParentheses::NeverPreserve. I'm going to think a bit more about some good variant names.

cnpryer
cnpryer commented Jul 20, 2023
View reviewed changes
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from 7078937 to d7b8d1d Compare July 20, 2023 01:18
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from d7b8d1d to 6e6bb89 Compare July 20, 2023 01:20
@MichaReiser
Copy link
Member

MichaReiser commented Jul 20, 2023

I'm not in love with TupleParentheses::NeverPreserve. I'm going to think a bit more about some good variant names.

@konstin likes the name a lot. I believe it has the same semantics as Parenthesize::IfBreaks (which naming I find okay, but not good)

MichaReiser
MichaReiser approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

This looks good to me. I've one outstanding question about what the expected behavior for lower priority (in_parentheses_only_group) split points is inside of comprehensions.

@cnpryer cnpryer force-pushed the chore-tuple-paren branch 2 times, most recently from 2cb21c3 to 9ed80da Compare July 20, 2023 16:28
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from 9ed80da to 97710d2 Compare July 20, 2023 16:30
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from 1b3178e to 9e65b9a Compare July 22, 2023 19:42
@cnpryer cnpryer force-pushed the chore-tuple-paren branch from a490515 to 4cdc684 Compare July 24, 2023 14:34
@MichaReiser MichaReiser enabled auto-merge (squash) July 24, 2023 14:41
@MichaReiser
Copy link
Member

MichaReiser commented Jul 24, 2023

Thank you. Enabling auto merge. Hopefully, this flushes through.

@MichaReiser MichaReiser merged commit 8eadacd into astral-sh:main Jul 24, 2023
@cnpryer cnpryer deleted the chore-tuple-paren branch July 24, 2023 14:46
@cnpryer cnpryer mentioned this pull request Jul 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@konstin konstin konstin left review comments

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.

3 participants

@cnpryer @MichaReiser @konstin