Use parentheses with equality check in walrus/assignment statements

[Shivansh-007]

Closes #449

Checklist - did you ...

• [X] Add a CHANGELOG entry if necessary?
• [X] Add / update tests if necessary?
• [ ] Add new / update outdated documentation?

[JelleZijlstra]

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 == isn't a valid assignment target, like this one in pandas:

-            is_last_column = len(columns) - 1 == i
+            is_last_column = (len(columns) - 1 == i)

(diff-shades commenting currently doesn't work yet, but you can manually download the artifacts from https://github.com/psf/black/actions/runs/1697124259.)

[Shivansh-007]

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 a = b == c where b and c can only be a single leaf, like system.platform() and not len(columns) - 1. This is pretty helpful:

-        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.

[JelleZijlstra]

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.

[felix-hilden]

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. foo().bar = 3 is just asking for trouble 😄

[github-actions[bot]]

diff-shades results comparing this PR (ce9e47d72712781f55025f32a514317ad31adb9d) to main (71e71e5f52e5f6bdeae63cc8c11b1bee44d11c30). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 2 projects & 8 files changed / 20 changes [+10/-10]    │
│                                                        │
│ ... out of 2 162 297 lines, 10 458 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

[JelleZijlstra]

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.)

[JelleZijlstra]

Why is this blocked? There are a lot of merge conflicts now. I vote that we close this and leave the style as is.

[ichard26]

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.