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

Support fmt: skip on compound statements #6593

Merged
merged 2 commits into from
Aug 17, 2023
Merged

Conversation

MichaReiser
Copy link
Member

Summary

This PR adds support for trailing fmt: skip comments suppressing the formatting of a compound statement's clause header (if, elif, try...)

Test Plan

I added new test cases

@MichaReiser
Copy link
Member Author

Comment on lines 40 to 43
if SuppressionKind::has_skip_comment(trailing_definition_comments, f.context().source()) {
SuppressedClauseHeader::Class(item).fmt(f)?;
} else {
write!(f, [text("class"), space(), name.format()])?;
Copy link
Member Author

@MichaReiser MichaReiser Aug 15, 2023

Choose a reason for hiding this comment

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

The pattern is always the same. Test if there's a trailing skip comment and, if so, call SuppressedClauseHeader. The enclosing node remains responsible for formatting the body and the trailing comments.

We could wrap this in a ClauseHeader struct but it would result in the same level of nesting because it would need to accept a Format object to write the content. It may be worth adding considering that @tjkuson needs it in #6592 to collapse ... bodies.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 15, 2023

PR Check Results

Benchmark

Linux

group                                      main                                    pr
-----                                      ----                                    --
formatter/large/dataset.py                 1.03      3.9±0.28ms    10.5 MB/sec     1.00      3.8±0.35ms    10.8 MB/sec
formatter/numpy/ctypeslib.py               1.01   776.0±37.22µs    21.5 MB/sec     1.00   769.0±70.03µs    21.7 MB/sec
formatter/numpy/globals.py                 1.00     77.8±7.67µs    37.9 MB/sec     1.06     82.2±5.34µs    35.9 MB/sec
formatter/pydantic/types.py                1.01  1481.6±220.30µs    17.2 MB/sec    1.00  1469.3±93.90µs    17.4 MB/sec
linter/all-rules/large/dataset.py          1.00     11.9±0.44ms     3.4 MB/sec     1.06     12.6±0.71ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.2±0.10ms     5.3 MB/sec     1.03      3.2±0.12ms     5.1 MB/sec
linter/all-rules/numpy/globals.py          1.00   454.6±18.04µs     6.5 MB/sec     1.06   482.9±41.58µs     6.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.2±0.22ms     4.1 MB/sec     1.06      6.6±0.33ms     3.8 MB/sec
linter/default-rules/large/dataset.py      1.00      6.5±0.32ms     6.2 MB/sec     1.01      6.6±0.31ms     6.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1344.7±55.15µs    12.4 MB/sec     1.06  1424.2±76.70µs    11.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    183.9±7.88µs    16.0 MB/sec     1.01   184.8±10.45µs    16.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.9±0.14ms     8.7 MB/sec     1.01      3.0±0.15ms     8.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.7±0.02ms    11.0 MB/sec    1.01      3.7±0.03ms    10.9 MB/sec
formatter/numpy/ctypeslib.py               1.00    739.7±7.58µs    22.5 MB/sec    1.01    747.6±9.00µs    22.3 MB/sec
formatter/numpy/globals.py                 1.00     77.2±1.14µs    38.2 MB/sec    1.01     78.2±2.17µs    37.7 MB/sec
formatter/pydantic/types.py                1.00  1514.5±14.79µs    16.8 MB/sec    1.02  1539.5±81.10µs    16.6 MB/sec
linter/all-rules/large/dataset.py          1.01     13.1±0.06ms     3.1 MB/sec    1.00     12.9±0.10ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.04ms     4.6 MB/sec    1.00      3.6±0.02ms     4.6 MB/sec
linter/all-rules/numpy/globals.py          1.00    380.0±6.09µs     7.8 MB/sec    1.00    378.8±5.13µs     7.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.8±0.06ms     3.7 MB/sec    1.00      6.8±0.11ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.02      7.2±0.05ms     5.6 MB/sec    1.00      7.1±0.04ms     5.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1488.4±10.67µs    11.2 MB/sec    1.00   1488.6±9.56µs    11.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    154.4±1.57µs    19.1 MB/sec    1.00    154.8±3.34µs    19.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.02ms     8.0 MB/sec    1.00      3.2±0.02ms     8.0 MB/sec

@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from ef86a00 to 8215d9d Compare August 15, 2023 14:20
@MichaReiser MichaReiser force-pushed the fmt-skip-clause-header branch 2 times, most recently from e008ba2 to b9667c7 Compare August 15, 2023 16:42
@MichaReiser
Copy link
Member Author

@charliermarsh The PR has two commits:

  1. Repeats the code to check whether there's a suppression comment. Plain and simple but with some repetitive code.
  2. Introduces a new clause_header formatter that checks the suppression and formatting the :, the trailing colon comment, and any potential leading comments.

I'm undecided whether 2 is over-engineer. Wdyt?

clause_header(
ClauseHeader::ExceptHandler(item),
dangling_comments,
&format_with(|f| {
Copy link
Member Author

Choose a reason for hiding this comment

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

The formatting of the case header has been moved into the format_with. The clause_header implementation now takes care of writing the : and the trailing colon comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

The ClauseHeader abstractin could potentially also be useful in placement.rs

@charliermarsh
Copy link
Member

Haha yeah, this is a close one, but I like what you have in the second commit, it doesn't feel over-engineered to me and is more declarative + feels very consistent with the other design patterns in the formatter.

@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 8215d9d to 247979d Compare August 15, 2023 17:05
@MichaReiser MichaReiser force-pushed the fmt-skip-clause-header branch 2 times, most recently from 89f9f04 to ca611a8 Compare August 15, 2023 17:11
@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 16, 2023
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 247979d to 142d47b Compare August 16, 2023 06:07
@MichaReiser MichaReiser force-pushed the fmt-skip-clause-header branch 2 times, most recently from 9fc3fd4 to 6fc6c13 Compare August 16, 2023 06:10
@MichaReiser MichaReiser marked this pull request as ready for review August 16, 2023 06:10
@MichaReiser MichaReiser mentioned this pull request Aug 15, 2023
6 tasks
@MichaReiser MichaReiser force-pushed the fmt-skip-clause-header branch 2 times, most recently from 47988c9 to f20577f Compare August 16, 2023 06:36
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 142d47b to e2de436 Compare August 17, 2023 05:49
Base automatically changed from fmt-skip-statement-and-decorators to main August 17, 2023 05:58
@MichaReiser MichaReiser force-pushed the fmt-skip-clause-header branch from f20577f to c8b4a9e Compare August 17, 2023 05:59
@MichaReiser MichaReiser enabled auto-merge (squash) August 17, 2023 05:59
@MichaReiser MichaReiser merged commit fa7442d into main Aug 17, 2023
@MichaReiser MichaReiser deleted the fmt-skip-clause-header branch August 17, 2023 06:05
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.

2 participants