Skip to content

Comments

Support fmt: skip for simple-statements and decorators#6561

Merged
MichaReiser merged 1 commit intomainfrom
fmt-skip-statement-and-decorators
Aug 17, 2023
Merged

Support fmt: skip for simple-statements and decorators#6561
MichaReiser merged 1 commit intomainfrom
fmt-skip-statement-and-decorators

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 14, 2023

Summary

This PR implements support for trailing fmt: skip and fmt: off comments on statement and decorator level.

This PR does not yet implement fmt: skip support at the end of a case header, e.g. at the end of an if statement

Test Plan

Matching black tests, added new tests

This was referenced Aug 14, 2023
Merged
Merged
Merged
@MichaReiser
Copy link
Member Author

MichaReiser commented Aug 14, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser mentioned this pull request Aug 14, 2023
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2023 •
edited
Loading

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.4±0.03ms    11.9 MB/sec    1.00      3.4±0.04ms    12.0 MB/sec
formatter/numpy/ctypeslib.py               1.00    710.6±2.65µs    23.4 MB/sec    1.01   714.3±10.55µs    23.3 MB/sec
formatter/numpy/globals.py                 1.00     75.7±0.31µs    39.0 MB/sec    1.00     75.8±0.34µs    38.9 MB/sec
formatter/pydantic/types.py                1.00  1393.2±23.61µs    18.3 MB/sec    1.00  1391.0±25.38µs    18.3 MB/sec
linter/all-rules/large/dataset.py          1.00     10.6±0.25ms     3.9 MB/sec    1.01     10.6±0.29ms     3.8 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.8±0.07ms     5.9 MB/sec    1.00      2.8±0.07ms     5.9 MB/sec
linter/all-rules/numpy/globals.py          1.01    397.9±1.60µs     7.4 MB/sec    1.00    394.9±1.71µs     7.5 MB/sec
linter/all-rules/pydantic/types.py         1.03      5.6±0.21ms     4.6 MB/sec    1.00      5.4±0.10ms     4.7 MB/sec
linter/default-rules/large/dataset.py      1.01      5.5±0.09ms     7.4 MB/sec    1.00      5.5±0.07ms     7.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1209.6±6.54µs    13.8 MB/sec    1.00   1212.7±6.77µs    13.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    143.8±3.91µs    20.5 MB/sec    1.00    143.8±5.33µs    20.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.02ms    10.3 MB/sec    1.00      2.5±0.04ms    10.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.7±0.04ms    10.9 MB/sec    1.01      3.8±0.24ms    10.8 MB/sec
formatter/numpy/ctypeslib.py               1.00   772.2±14.12µs    21.6 MB/sec    1.00   769.0±15.21µs    21.7 MB/sec
formatter/numpy/globals.py                 1.00     80.5±1.37µs    36.6 MB/sec    1.02     81.9±3.27µs    36.0 MB/sec
formatter/pydantic/types.py                1.00  1550.2±34.30µs    16.5 MB/sec    1.00  1555.0±37.94µs    16.4 MB/sec
linter/all-rules/large/dataset.py          1.00     12.8±0.19ms     3.2 MB/sec    1.00     12.8±0.21ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.04ms     4.7 MB/sec    1.00      3.5±0.05ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.01    445.7±7.51µs     6.6 MB/sec    1.00    440.5±7.69µs     6.7 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.6±0.10ms     3.8 MB/sec    1.00      6.6±0.19ms     3.8 MB/sec
linter/default-rules/large/dataset.py      1.01      7.1±0.10ms     5.8 MB/sec    1.00      7.0±0.06ms     5.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02  1505.6±29.53µs    11.1 MB/sec    1.00  1482.6±21.01µs    11.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    175.6±3.39µs    16.8 MB/sec    1.00    175.9±4.73µs    16.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.04ms     8.1 MB/sec    1.00      3.1±0.03ms     8.1 MB/sec

@MichaReiser MichaReiser force-pushed the fix-suppressed-indentation branch from 12f9bbc to bdab1c2 Compare August 14, 2023 15:47
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 05c1442 to 3eb2b08 Compare August 14, 2023 15:47
@MichaReiser MichaReiser force-pushed the fix-suppressed-indentation branch from bdab1c2 to df98068 Compare August 14, 2023 16:09
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 3eb2b08 to 8731120 Compare August 14, 2023 16:09
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 8731120 to 5f5873c Compare August 14, 2023 16:35
@MichaReiser MichaReiser mentioned this pull request Aug 15, 2023
Merged
Base automatically changed from fix-suppressed-indentation to main August 15, 2023 06:00
@MichaReiser MichaReiser changed the base branch from main to comments-single-lookup August 15, 2023 06:06
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch 2 times, most recently from f40f7c9 to 5bc64b5 Compare August 15, 2023 06:11
@MichaReiser MichaReiser marked this pull request as ready for review August 15, 2023 06:12
@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 15, 2023
@MichaReiser MichaReiser mentioned this pull request Aug 15, 2023
Closed
6 tasks
MichaReiser
MichaReiser commented Aug 15, 2023
View reviewed changes
crates/ruff_python_formatter/src/lib.rs
self.fmt_node(node, f)?;
self.fmt_dangling_comments(node_comments.dangling, f)?;
trailing_comments(node_comments.trailing).fmt(f)
if self.is_suppressed(node_comments.trailing, f.context()) {
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.

I considered handling the suppression in the FormatStmt so that it isn't necessary to override is_suppressed for all statements. However, it requires that FormatStmt resolves the trailing comments to test for a suppression comment, just for the FormatNodeRule to lookup the very same comments again.

@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 5bc64b5 to 02330c4 Compare August 15, 2023 11:53
@MichaReiser MichaReiser changed the title Support fmt: skip on statement and decorator level Support fmt: skip for simple-statements and decorator level Aug 15, 2023
@MichaReiser MichaReiser changed the title Support fmt: skip for simple-statements and decorator level Support fmt: skip for simple-statements and decorators Aug 15, 2023
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 02330c4 to ef86a00 Compare August 15, 2023 11:54
@MichaReiser MichaReiser mentioned this pull request Aug 15, 2023
Merged
@MichaReiser MichaReiser force-pushed the comments-single-lookup branch from be450a6 to 072263c Compare August 15, 2023 14:20
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from ef86a00 to 8215d9d Compare August 15, 2023 14:20
Base automatically changed from comments-single-lookup to main August 15, 2023 15:39
@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch 2 times, most recently from 247979d to 142d47b Compare August 16, 2023 06:07
charliermarsh
charliermarsh approved these changes Aug 16, 2023
View reviewed changes
crates/ruff_python_formatter/src/comments/placement.rs
class_def: &'a ast::StmtClassDef,
) -> CommentPlacement<'a> {
if comment.start() < class_def.name.start() {
if comment.line_position().is_own_line() && comment.start() < class_def.name.start() {
Copy link
Member

@charliermarsh charliermarsh Aug 16, 2023

Choose a reason for hiding this comment

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

End-of-line comments are already correctly associated, I'm guessing, since they're associated with the preceding rather than the following node?

Copy link
Member Author

@MichaReiser MichaReiser Aug 17, 2023

Choose a reason for hiding this comment

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

Yes. The default placement correctly associates them with the decorator (preceding) node.

@MichaReiser MichaReiser force-pushed the fmt-skip-statement-and-decorators branch from 142d47b to e2de436 Compare August 17, 2023 05:49
@MichaReiser MichaReiser enabled auto-merge (squash) August 17, 2023 05:51
@MichaReiser MichaReiser merged commit 4dc32a0 into main Aug 17, 2023
@MichaReiser MichaReiser deleted the fmt-skip-statement-and-decorators branch August 17, 2023 05:58
@MichaReiser MichaReiser mentioned this pull request Aug 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charliermarsh charliermarsh charliermarsh 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.

2 participants

@MichaReiser @charliermarsh