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

Formatter: Unstable call arguments comment formatting #6818

Closed
MichaReiser opened this issue Aug 23, 2023 · 6 comments · Fixed by #6826
Closed

Formatter: Unstable call arguments comment formatting #6818

MichaReiser opened this issue Aug 23, 2023 · 6 comments · Fixed by #6826
Assignees
Labels
bug Something isn't working formatter Related to the formatter

Comments

@MichaReiser
Copy link
Member

I created this example to test some edge cases with #6817. That PR is handling the edge case correctly but I can't add the test case because the call expression formatting with the following comment itself is unstable

aaa = (
    bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    # awkward comment
    ()
    .bbbbbbbbbbbbbbbb
)


aaa = bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb# awkward comment
().bbbbbbbbbbbbbbbb

Playground

@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Aug 23, 2023
@MichaReiser MichaReiser added this to the Formatter: Beta milestone Aug 23, 2023
@charliermarsh
Copy link
Member

@MichaReiser - Want me to look at this, or are you taking as part of #6817?

@MichaReiser
Copy link
Member Author

MichaReiser commented Aug 23, 2023

@MichaReiser - Want me to look at this, or are you taking as part of #6817?

I'm not solving it. I used a different test case to work around the issue.

@charliermarsh
Copy link
Member

I'm thinking that I'll just make that a leading comment on bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.

@MichaReiser
Copy link
Member Author

Prettier makes it a leading comment of what comes after Playground. You have to be careful with comment ordering if you make it a leading comment of bbb, e.g if bbb has some trailing comments too

@charliermarsh
Copy link
Member

I need to understand why we break the latter but not the former:

aaa = (
    bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    # awkward comment
    ()
    .bbbbbbbbbbbbbbbb
)

aaa = (
    bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
    ()
    # awkward comment
    .bbbbbbbbbbbbbbbb
)

@MichaReiser
Copy link
Member Author

Attribute formatting has special handling for leading comments of the next attribute

(!leading_attribute_comments.is_empty()).then_some(hard_line_break()),
leading_comments(leading_attribute_comments),
text("."),
trailing_comments(trailing_dot_comments),
attr.format()

@charliermarsh charliermarsh self-assigned this Aug 23, 2023
charliermarsh added a commit that referenced this issue Aug 25, 2023
## Summary

We now format comments between a function and its arguments as dangling.
Like with other strange placements, I've biased towards preserving the
existing formatting, rather than attempting to reorder the comments.

Closes #6818.

## Test Plan

`cargo test`

Before:

| project      | similarity index |
|--------------|------------------|
| cpython      | 0.76050          |
| django       | 0.99820          |
| transformers | 0.99800          |
| twine        | 0.99876          |
| typeshed     | 0.99953          |
| warehouse    | 0.99615          |
| zulip        | 0.99729          |

After:

| project      | similarity index |
|--------------|------------------|
| cpython      | 0.76050          |
| django       | 0.99820          |
| transformers | 0.99800          |
| twine        | 0.99876          |
| typeshed     | 0.99953          |
| warehouse    | 0.99615          |
| zulip        | 0.99729          |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter Related to the formatter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants