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

Track formatted comments #4979

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Track formatted comments #4979

merged 2 commits into from
Jun 9, 2023

Conversation

MichaReiser
Copy link
Member

Summary

This PR removes the debug-only condition from SourceComment::formatted and now uses it to track whether a comment has already been formatted and,
if so, skip it during the leading/dangling/trailing comments formatting. The motivation for this change is that it is sometimes necessary that
the parent node formats the comments and we then need a way to prevent that the FormatNodeRule formats the same comments again. One such example
is the boolean operation formatting:

if (
    self._proc is not None
    # has the child process finished?
    and self._returncode is None
    # the child process has finished, but the
    # transport hasn't been notified yet?
    and self._proc.poll() is None
):
    pass

You can see that the leading comments of the right hand side must be formatted before the operator but the default comment formatting
formats them after. Another use case is that the leading comments of a *args or **kwargs must be formatted before the * or **.

The *args and **kwargs use case used a one-off solution so far which was easy enough to implement because it only applies to Arg. Doing the same
with boolean operations isn't as straightforward because the value can be an arbitrary Expr.

Test Plan

cargo test (the *args and **kwargs formatting still work as expected without any explicit handling)

Performance

I don't expect this to change performance much because the overhead is per comment and programs often only have few comments

@MichaReiser MichaReiser changed the title Track formatted comment Track formatted comments Jun 9, 2023
@MichaReiser MichaReiser added internal An internal refactor or improvement formatter Related to the formatter labels Jun 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.0±0.04ms     5.8 MB/sec    1.02      7.2±0.11ms     5.7 MB/sec
formatter/numpy/ctypeslib.py               1.00   1416.9±2.37µs    11.8 MB/sec    1.01   1433.2±3.10µs    11.6 MB/sec
formatter/numpy/globals.py                 1.00    139.3±0.21µs    21.2 MB/sec    1.01    141.0±0.28µs    20.9 MB/sec
formatter/pydantic/types.py                1.00      2.9±0.02ms     8.9 MB/sec    1.01      2.9±0.01ms     8.8 MB/sec
linter/all-rules/large/dataset.py          1.00     15.3±0.15ms     2.7 MB/sec    1.00     15.3±0.19ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.04ms     4.6 MB/sec    1.00      3.7±0.10ms     4.6 MB/sec
linter/all-rules/numpy/globals.py          1.01    370.0±1.10µs     8.0 MB/sec    1.00    367.2±1.46µs     8.0 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.4±0.12ms     4.0 MB/sec    1.00      6.3±0.10ms     4.0 MB/sec
linter/default-rules/large/dataset.py      1.02      7.6±0.06ms     5.4 MB/sec    1.00      7.4±0.08ms     5.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02   1569.8±3.63µs    10.6 MB/sec    1.00   1539.3±4.47µs    10.8 MB/sec
linter/default-rules/numpy/globals.py      1.02    167.7±0.23µs    17.6 MB/sec    1.00    164.7±0.31µs    17.9 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.4±0.01ms     7.6 MB/sec    1.00      3.3±0.03ms     7.7 MB/sec

Windows

group                                      main                                    pr
-----                                      ----                                    --
formatter/large/dataset.py                 1.18      9.5±2.47ms     4.3 MB/sec     1.00      8.0±0.14ms     5.1 MB/sec
formatter/numpy/ctypeslib.py               1.03  1682.3±187.10µs     9.9 MB/sec    1.00  1626.8±32.79µs    10.2 MB/sec
formatter/numpy/globals.py                 1.00    153.7±4.08µs    19.2 MB/sec     1.01    154.7±6.50µs    19.1 MB/sec
formatter/pydantic/types.py                1.00      3.2±0.06ms     7.9 MB/sec     1.01      3.3±0.05ms     7.8 MB/sec
linter/all-rules/large/dataset.py          1.00     18.0±0.31ms     2.3 MB/sec     1.00     18.0±0.26ms     2.3 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.4±0.08ms     3.8 MB/sec     1.00      4.4±0.08ms     3.8 MB/sec
linter/all-rules/numpy/globals.py          1.00   514.8±12.64µs     5.7 MB/sec     1.00   514.2±10.72µs     5.7 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.5±0.16ms     3.4 MB/sec     1.00      7.5±0.14ms     3.4 MB/sec
linter/default-rules/large/dataset.py      1.03      9.0±0.66ms     4.5 MB/sec     1.00      8.7±0.13ms     4.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.06  1924.0±162.96µs     8.7 MB/sec    1.00  1818.8±29.21µs     9.2 MB/sec
linter/default-rules/numpy/globals.py      1.06   217.6±48.57µs    13.6 MB/sec     1.00    204.8±8.10µs    14.4 MB/sec
linter/default-rules/pydantic/types.py     1.08      4.2±0.60ms     6.1 MB/sec     1.00      3.9±0.06ms     6.6 MB/sec

@MichaReiser MichaReiser requested a review from konstin June 9, 2023 07:18
crates/ruff_python_formatter/src/comments/mod.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser force-pushed the fix-bin-oip-with-leading-comments branch from 4cf24dc to 7458b5b Compare June 9, 2023 08:46
@MichaReiser MichaReiser force-pushed the skip-formatted-comments branch from 48356d4 to 192261b Compare June 9, 2023 08:46
@MichaReiser MichaReiser force-pushed the fix-bin-oip-with-leading-comments branch from 7458b5b to e341c1c Compare June 9, 2023 08:56
Base automatically changed from fix-bin-oip-with-leading-comments to main June 9, 2023 09:02
@MichaReiser MichaReiser force-pushed the skip-formatted-comments branch from ca58055 to f444677 Compare June 9, 2023 09:04
@MichaReiser MichaReiser enabled auto-merge (squash) June 9, 2023 09:04
@MichaReiser MichaReiser merged commit 68d52da into main Jun 9, 2023
@MichaReiser MichaReiser deleted the skip-formatted-comments branch June 9, 2023 09:09
konstin pushed a commit that referenced this pull request Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants