-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Use parentheses with equality check in walrus/assignment statements #2770
Use parentheses with equality check in walrus/assignment statements #2770
Conversation
Looking at the results of this change, I'm not sure it's really worth it. The diff shows that this is a pretty common pattern, so this would be a relatively disruptive change. To me, most of the changes don't seem like clear improvements, so I'd default to sticking to Black's existing style. (Relatedly, the original issue got more downvote reactions than upvotes.) The parentheses seem especially unnecessary in cases where the left operand of the
(diff-shades commenting currently doesn't work yet, but you can manually download the artifacts from https://github.com/psf/black/actions/runs/1697124259.) |
Sorry, I was looking to edit my previous comment, but misclicked delete instead. I agree with you but in cases where the expression is in form of - left_ea = blk_vals.ndim == 1
+ left_ea = (blk_vals.ndim == 1)
- result = arr == other
+ result = (arr == other)
- actual = self.index.values == self.index
+ actual = (self.index.values == self.index) But still, I am not sure about cases like, where they are in the above format but are looking better without the parentheses: - result = Series(a1, dtype=cat_type) == Series(a2, dtype=cat_type)
+ result = (Series(a1, dtype=cat_type) == Series(a2, dtype=cat_type))
- mask &= (self._ascending_count - start) % step == 0
+ mask &= ((self._ascending_count - start) % step == 0)
- debug = env.get(str("_VIRTUALENV_PERIODIC_UPDATE_INLINE")) == str("1")
+ debug = (env.get(str("_VIRTUALENV_PERIODIC_UPDATE_INLINE")) == str("1")) I think this can still be discussed a bit more on the issue with the original commenters. |
To be clear my current preference is just to never add these parentheses, it's just that the parens are more jarring in cases where the LHS of the == is complex. I'm OK with landing a version of this change if there's consensus for it though. |
I tend to always add the parens, but this isn't the hill I'm willing to die on. A good middle ground could be using a definition of "complex" like in our power hugging endeavour. It's not so easy to determine if something can be used as an assignment target, but a combination of name-attribute-subscript is a good approximation. |
6984931
to
32e5544
Compare
diff-shades results comparing this PR (ce9e47d) to main (71e71e5). The full diff is available in the logs under the "Generate HTML diff report" step.
|
This will now need to go in the preview style only. (And my personal preference is still to just not change anything here, but the preview style does allow us to change our mind more easily.) |
…gnment-parentheses
434d8bf
to
0aee688
Compare
Why is this blocked? There are a lot of merge conflicts now. I vote that we close this and leave the style as is. |
I don't remember well. I think I marked it as blocked because it wasn't even clear if the style change it was implementing was accepted (the PR is blocked until the style decision is made). Made sense in my head when I added the label, but I agree it's confusing in hindsight. |
Closes #449
Checklist - did you ...