-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Avoid parenthesizing unsplittable because of comments #8431
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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