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 instability: hard line break in binary expression #7605

Closed
charliermarsh opened this issue Sep 22, 2023 · 6 comments · Fixed by #7693
Closed

Formatter instability: hard line break in binary expression #7605

charliermarsh opened this issue Sep 22, 2023 · 6 comments · Fixed by #7693
Assignees
Labels
accepted Ready for implementation bug Something isn't working formatter Related to the formatter

Comments

@charliermarsh
Copy link
Member

charliermarsh commented Sep 22, 2023

Given:

[
    1 for components in  # pylint: disable=undefined-loop-variable
    b +  # integer 1 may only have decimal 01-09
    c  # negative decimal
] 

We format as:

[
    1
    for components in b  # pylint: disable=undefined-loop-variable  # integer 1 may only have decimal 01-09
    +
    c  # negative decimal
]

(Notice the hard line break between + and c # negative decimal.)

Which is then formatted as:

[
    1
    for components in b  # pylint: disable=undefined-loop-variable  # integer 1 may only have decimal 01-09
    + c  # negative decimal
]

Sourced from #7590 (A_1798643049678049848.py).

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

I updated the summary with a smaller reproduction

@charliermarsh
Copy link
Member Author

Thanks.

@charliermarsh
Copy link
Member Author

Didn't mean to assign to myself 😂

@MichaReiser
Copy link
Member

MichaReiser commented Sep 27, 2023

I think this is caused by #6588

The binary expression can have dangling comments and it formats them manually. But it's the comprehension that attaches a dangling comment to the binary expression, which messes up the formatting logic, which it intends to handle but the binary expression formatting can't know that.

The solution here is that comprehension shouldn't attach dangling comments to other nodes.

@charliermarsh
Copy link
Member Author

I can fix that.

@MichaReiser
Copy link
Member

MichaReiser commented Sep 27, 2023

The problematic comment placement are:

if comment.start() < comprehension.iter.start() {
return if is_own_line {
CommentPlacement::Default(comment)
} else {
// after the `in` but same line, turn into trailing on the `in` token
CommentPlacement::dangling(&comprehension.iter, comment)
};
}

Not related to this issue but has the same underlying problem

if is_own_line {
if last_end < comment.start() && comment.start() < if_token.start() {
return CommentPlacement::dangling(if_node, comment);
}
} else if if_token.start() < comment.start() && comment.start() < if_node.start() {
return CommentPlacement::dangling(if_node, comment);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation 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