Prefer more equal signs before a break when splitting chained assignments

[henriholopainen]

Description

This PR makes rhs processing prefer more equal signs before breaking the line.

Fixes #4007

Checklist - did you ...

• Add an entry in CHANGES.md if necessary?
• Add / update tests if necessary?

[github-actions]

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

╭─────────────────────── Summary ────────────────────────╮
│ 2 projects & 19 files changed / 176 changes [+86/-90]  │
│                                                        │
│ ... out of 2 522 686 lines, 11 793 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

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

[MichaReiser]

Thanks

The following diff seems interesting

         elif constraint is None:
-            self.constraint_target = (
-                self.inferred_target_elements
-            ) = self.inferred_target_whereclause = None
+            self.constraint_target = self.inferred_target_elements = (
+                self.inferred_target_whereclause
+            ) = None

But I guess there's not much we can do about (we need the parentheses because it's python)

[JelleZijlstra]

I'm not quite sure about this. Looking at the diff-shades output, there's a few good ones:

-                            collected_attributes[name] = column_copies[
-                                obj
-                            ] = ret
+                            collected_attributes[name] = column_copies[obj] = (
+                                ret
+                            )

It's nicer to keep the subscript on one line.

-        self.selectable = (
-            self.persist_selectable
-        ) = self.local_table = selectable
+        self.selectable = self.persist_selectable = self.local_table = (
+            selectable
+        )

It's better to keep all the assignment targets next to each other, instead of randomly enclosing one in parentheses.

But there's also a lot of cases like the one @MichaReiser highlighted, where we just change which of the LHS assignment targets get parenthesized. In those cases, I don't think either formatting is clearly better; it looks ugly either way. I'd rather have Black not make a change there, because every time we change the formatting of existing code, that's disruptive to users. I'd want to make changes only if we believe the new formatting is better. Therefore, I'd prefer to stick with the current formatting in those cases.

Perhaps the rule could be that we split around the RHS if possible, and if that doesn't work, we preserve the current behavior and split around the second assignment target from the left.

[henriholopainen]

I'm not quite sure about this. Looking at the diff-shades output, there's a few good ones:

-                            collected_attributes[name] = column_copies[
-                                obj
-                            ] = ret
+                            collected_attributes[name] = column_copies[obj] = (
+                                ret
+                            )

It's nicer to keep the subscript on one line.

-        self.selectable = (
-            self.persist_selectable
-        ) = self.local_table = selectable
+        self.selectable = self.persist_selectable = self.local_table = (
+            selectable
+        )

It's better to keep all the assignment targets next to each other, instead of randomly enclosing one in parentheses.

But there's also a lot of cases like the one @MichaReiser highlighted, where we just change which of the LHS assignment targets get parenthesized. In those cases, I don't think either formatting is clearly better; it looks ugly either way. I'd rather have Black not make a change there, because every time we change the formatting of existing code, that's disruptive to users. I'd want to make changes only if we believe the new formatting is better. Therefore, I'd prefer to stick with the current formatting in those cases.

Perhaps the rule could be that we split around the RHS if possible, and if that doesn't work, we preserve the current behavior and split around the second assignment target from the left.

For me even @MichaReiser's example is better with the new formatting. Somehow it feels more natural when as many assignments as possible end up formatted as early as possible. Consider for example this (rather extreme) example:

self.selectable = self.persist_selectable = self.local_table = self.foo_bar = self.foo = self.longlonglong_key = self.another_long_long_long_key = self.yet_again_a_long_key = value

On current main it becomes:

self.selectable = (
    self.persist_selectable
) = (
    self.local_table
) = (
    self.foo_bar
) = (
    self.foo
) = (
    self.longlonglong_key
) = self.another_long_long_long_key = self.yet_again_a_long_key = value

and with this PR:

self.selectable = self.persist_selectable = self.local_table = self.foo_bar = (
    self.foo
) = self.longlonglong_key = self.another_long_long_long_key = (
    self.yet_again_a_long_key
) = value

I don't think a hybrid model would make sense here:

self.selectable = (
    self.persist_selectable
) = (
    self.local_table
) = (
    self.foo_bar
) = (
    self.foo
) =  self.longlonglong_key = self.another_long_long_long_key = self.yet_again_a_long_key = (
    value
)

I think there is some inherent value to consistency and easy to follow rules. Thus I feel like we should stick with either splitting early or late, but not make it depend on the case, and in general it seems splitting late >= splitting early. And while I do appreciate avoiding introducing unnecessary diffs, this is quite the corner case and doesn't touch that many lines of code.

All that being said, as a contributor I trust your view on what is good and desired. Also, even though intuitively I feel consistency should weigh in cases where two formatting options are both ugly, I'm not fully sure if it should be enough to justify changing the formatting.

[JelleZijlstra]

@hauntsaninja do you have any opinion here? I think this is mostly an improvement but I'm a bit hesitant to make style changes in existing code that aren't clear improvements.

[hauntsaninja]

I think it's an improvement! Splitting the one in the middle is not usually a choice a human would make.
Moreover, the change here isn't super common or controversial, so I think churn costs aren't too high.

[JelleZijlstra]

Sounds good! Unfortunately diff-shades failed again, I'll retry to see if it succeeds.

[JelleZijlstra]

Let's do it then, I'd like to see if I can get diff-shades to work first though.

[JelleZijlstra]

I think the diff-shades errors miraculously fixed themselves.