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: Allow comments between and/or and the right hand side #6062

Closed
Tracked by #6069
konstin opened this issue Jul 25, 2023 · 11 comments · Fixed by #7269
Closed
Tracked by #6069

Formatter: Allow comments between and/or and the right hand side #6062

konstin opened this issue Jul 25, 2023 · 11 comments · Fixed by #7269
Assignees
Labels
formatter Related to the formatter

Comments

@konstin
Copy link
Member

konstin commented Jul 25, 2023

The following input is stable with black ...

if (
    a
    # own line comment between left hand side and `and`
    and
    # own line comment between `and` and right hand side
    b
):
    pass

... but we move the second own line comment before the and:

if (
    a
    # own line comment between left hand side and `and`
    # own line comment between `and` and right hand side
    and b
):
    pass

The behavior is the same with or, but + works, i.e. other binary expressions work but not boolean expressions.

@konstin konstin added the formatter Related to the formatter label Jul 25, 2023
@MichaReiser
Copy link
Member

We may want to generalize the formatting of all BinaryLike nodes to achieve consistent formatting.

@MichaReiser
Copy link
Member

I actually prefer our formatting over black's and it preserves the comment ordering. But we should align the comment placement for all binary like expressions.

@zanieb
Copy link
Member

zanieb commented Aug 16, 2023

I agree with Micha that our formatting is more readable at the expense of possibly overriding user intent. I don't see what extra context can be given by having the comment after the operator though.

@MichaReiser
Copy link
Member

MichaReiser commented Aug 17, 2023

I guess the challenge with the current formatting is how to handle trailing and comments and preserve the comment line position and order: Playground

I think my preferred formatting is:

if (
    a
    # own line comment between left hand side and `and`
    # trailing
    # own line comment between `and` and right hand side
    and b
):
    pass

It preserves the order, but changes trailing to an own linecomment. But I think this overall looks better.

Edit: Similar issue #6647

@MichaReiser
Copy link
Member

And this is another funny one CC: @charliermarsh You best run our formatter on django and take a look at the diff. There are plenty of examples.

@charliermarsh charliermarsh self-assigned this Aug 30, 2023
@charliermarsh
Copy link
Member

I can take it on.

@MichaReiser
Copy link
Member

Thank you @charliermarsh

If possible, could we align the comment handling with the binary expression formatting? It's very likely that I have to unify the two implementations and that would be significantly easier if they roughly work the same.

@MichaReiser
Copy link
Member

It turns out that the string formatting doesn't overlap with and or formatting. It only affects binary and comparison expressions. That means this is still up for grabs.

Overall, the binary / compare formatting seems very reasonable. Maybe we can mimick that formatting for boolean expressions

@MichaReiser
Copy link
Member

MichaReiser commented Sep 7, 2023

@charliermarsh do you plan to work on this next week? If not, @konstin would you be interested in taking a look. It's our biggest difference in Django by now or I'll try to have a look since it's related to binary expression formatting.

@charliermarsh charliermarsh removed their assignment Sep 7, 2023
@charliermarsh
Copy link
Member

@konstin - please go ahead! Haven’t started on it at all.

@konstin konstin self-assigned this Sep 8, 2023
@MichaReiser
Copy link
Member

@konstin sorry for the forth and back. I consider rewriting the boolean expression formatting to use the BinaryLike formatting (the same logic as the binary and compare expressions use) to fix #7004 just once rather than in two places. Maybe... just maybe, it fixes this difference too

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 a pull request may close this issue.

4 participants