-
Notifications
You must be signed in to change notification settings - Fork 2.7k
wrap_comprehension_in: Always wrap if there's delimiters #4881
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: cobalt <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: cobalt <[email protected]>
|
diff-shades results comparing this PR (1e0a8b4) to main (1b342ef). The full diff is available in the logs under the "Generate HTML diff report" step. |
hauntsaninja
left a comment
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.
(not a review of the code, just a review of the style)
Thanks for working on this!
I ran this on my work codebase and liked it there. I like the diff-shades on net too, but relatively more of those feels gratuitous.
Given that it does add more changes, it's late in the year, that there's something about adding parens here that feels a little new and different for Black (at least to me), and that the implementation is a little fiddlier, what do you say about the following:
- We ship
wrap_comprehension_inas it is on main for 2026- unless other people chime in on the issue or you have a point of view
- We experiment with this for 2027
|
I agree that this change probably shouldn't be made for v26, and we should either ship |
Signed-off-by: cobalt <[email protected]>
|
For what it's worth, in the two examples in the OP I feel the extra parentheses make the formatting worse. In the diff-shades output there are some changes I like, but I find myself disliking most of the ones where the in clause has an operator like My ideal would be that we add parentheses only if needed to split the clause over multiple lines, remove parentheses only if the clause consists of a single extremely simple node (e.g., a single name, literal, or call), and otherwise preserve the user's decision. Given that what we have on main right now arguably makes the code worse in some cases (#4877) and this proposed change is also not always clearly better, I agree it's better to back out this change from the stable style for this year. Hopefully for next year we can then get a better version in place. |
Description
Background info: On current stable, Black does not add nor remove parens around
inclauses in comprehensions at all.wrap_comprehension_instandardized it to always add them if it would then fit in the line limit, and to remove them otherwise.Resolves #4877
My approach was to always wrap the


inclause if there's delimiters in it (not counting dots). This resulted in changes like this in Black's codebase, which I think are better?I'm not sure if this change is best. It makes
wrap_comprehension_inmuch more intrusive, at the very least. Will have to look at shades.cc @hauntsaninja
Checklist - did you ...
--previewstyle, following thestability policy?
CHANGES.mdif necessary?