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.

Sign up for GitHub

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

Jump to bottom

Formatter instability: Leading clause header comments with preceding function #7465

Closed
MichaReiser opened this issue Sep 17, 2023 · 1 comment · Fixed by #7556
Closed

Formatter instability: Leading clause header comments with preceding function #7465

MichaReiser opened this issue Sep 17, 2023 · 1 comment · Fixed by #7556
Assignees
@charliermarsh
Labels
bug Something isn't working formatter Related to the formatter
Milestone
Formatter: Beta

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Sep 17, 2023 •
edited
Loading

Input

try:
    def test():
        pass
    #import chardet.constants
    #chardet.constants._debug = 1
except ImportError:
    pass

Format 1

try:

    def test():
        pass

    # import chardet.constants
    # chardet.constants._debug = 1
except ImportError:
    pass

Format 2

try:

    def test():
        pass

    # import chardet.constants
    # chardet.constants._debug = 1

except ImportError:
    pass

Sourced by #7455
@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Sep 17, 2023
@MichaReiser MichaReiser added this to the Formatter: Beta milestone Sep 17, 2023
@charliermarsh charliermarsh self-assigned this Sep 20, 2023
@charliermarsh
Copy link
Member

Think I figured this one out, and also identified a related deviation.

@charliermarsh charliermarsh mentioned this issue Sep 20, 2023
Merged
charliermarsh added a commit that referenced this issue Sep 21, 2023
@charliermarsh
Fix instability in trailing clause body comments (#7556)
124d95d 
## Summary

When we format the trailing comments on a clause body, we check if there
are any newlines after the last statement; if not, we insert one.

This logic didn't take into account that the last statement could itself
have trailing comments, as in:

```python
if True:
    pass

    # comment
else:
    pass
```

We were thus inserting a newline after the comment, like:

```python
if True:
    pass

    # comment

else:
    pass
```

In the context of function definitions, this led to an instability,
since we insert a newline _after_ a function, which would in turn lead
to the bug above appearing in the second formatting pass.

Closes #7465.

## Test Plan

`cargo test`

Small improvement in `transformers`, but no regressions.

Before:

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1631 |
| django | 0.99983 | 2760 | 36 |
| transformers | 0.99956 | 2587 | 404 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99983 | 3496 | 18 |
| warehouse | 0.99967 | 648 | 15 |
| zulip | 0.99972 | 1437 | 21 |

After:

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1631 |
| django | 0.99983 | 2760 | 36 |
| **transformers** | **0.99957** | **2587** | **402** |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99983 | 3496 | 18 |
| warehouse | 0.99967 | 648 | 15 |
| zulip | 0.99972 | 1437 | 21 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charliermarsh charliermarsh

Labels
bug Something isn't working formatter Related to the formatter
Projects
None yet
Milestone
Formatter: Beta
Development

Successfully merging a pull request may close this issue.

Fix instability in trailing clause body comments astral-sh/ruff
2 participants
@MichaReiser @charliermarsh