Avoid parenthesizing unsplittable because of comments#8431
Conversation
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
|
9ca1296 to
dc83f99
Compare
dc83f99 to
9c534b1
Compare
9c534b1 to
cb44b2f
Compare
cb44b2f to
fb8fc22
Compare
|
|
I like the new style |
| if_group_breaks(&inline_comments) | ||
| .with_group_id(Some(group_id)) | ||
| .fmt(f)?; | ||
| } |
There was a problem hiding this comment.
I'm trying to understand the relationship and expectations between this and the if !inline_comments.is_empty() { below. Could this not be moved outside of the closure?
There was a problem hiding this comment.
The inine_comments.is_empty is just an optimization so that we don't write the if_group_breaks and call into trailing_comments when the list is empty. It doesn't change the formatted result.
Could this not be moved outside of the closure?
If you mean the !inline_comments.is_empty then no, because we always want to apply the best_fit_parenthesize style. What we could do is to duplicate the best_fit_parenthesize instead. No strong opinion on either of them.
| // attribute chains and non-fluent call expressions. See https://github.com/psf/black/issues/4001#issuecomment-1786681792 | ||
| // | ||
| // This logic isn't implemented in [`place_comment`] by associating trailing statement comments to the expression because | ||
| // doing so breaks the suite empty lines formatting that relies on trailing comments to be stored on the statement. |
There was a problem hiding this comment.
Would this work out-of-the-box if we did associate the trailing statement comments on the expression, ignoring the suite formatting?
There was a problem hiding this comment.
No, it would still require the if_group_breaks and if_group_fits but it would avoid the unprecedented "the child steals comments from its parent".
charliermarsh
left a comment
There was a problem hiding this comment.
I haven't reviewed the ecosystem changes in detail, but the PR and summary look right to me.

Summary
This PR implements a fix for #8041 where Ruff parenthesized unsplittable values in assignments if it has a trailing end-of-line comment that makes the assignment go over the line length.
The way black solves this is by moving the trailing end-of-line comment into the assignment so that it only parenthesizes the value (and comment) if formatting it on a new line makes both fit.
Closes #8041
Notes
There's one downside to this approach. It violates our principle of never collapsing an expression if it has trailing line comments. Let's take this example:
It is now formatted as
To ensure the new comment style is reversible.
Non-Fluent attribute chains
Non-fluent attribute chains aren't splittable. However, Black formats the comments after the closing parentheses as ruff used to before this change (related #8182). I decided to favour Black compatibility over consistence because it introduces about 30 file regressions in the similarity index check. Which is more than this PR fixes.
Test Plan
I added extensive tests and reviewed the ecosystem changes. They all seem to capture better the user's intent (and e.g. avoid moving pragma comments.
Main
PR