Skip to content
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

Prefer splitting right hand side of assignment statements. #3368

Merged
merged 10 commits into from Dec 15, 2022

Conversation

yilei
Copy link
Contributor

@yilei yilei commented Oct 31, 2022

Description

When an assignment statement's left hand side fits on a single line, prefer splitting the right hand side's outer optional parens.

Fixes #1498.

Checklist - did you ...

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

@github-actions
Copy link

github-actions bot commented Oct 31, 2022

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

╭──────────────────────── Summary ─────────────────────────╮
│ 11 projects & 66 files changed / 750 changes [+376/-374] │
│                                                          │
│ ... out of 2 355 274 lines, 10 989 files & 23 projects   │
╰──────────────────────────────────────────────────────────╯

Differences found.

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

@yilei yilei marked this pull request as draft October 31, 2022 23:18
@yilei
Copy link
Contributor Author

yilei commented Oct 31, 2022

Looking at the diff-shades results, I need to fix the following isuses:

  1. Trailing comma in a RHS tuple isn't handled:
-        (
-            field,
-            targets,
-            alias,
-            joins,
-            path,
-            opts,
-            transform_function,
-        ) = self._setup_joins(pieces, opts, alias)
+        (field, targets, alias, joins, path, opts, transform_function,) = (
+            self._setup_joins(pieces, opts, alias)
+        )
  1. There are some unrelated changes where the RHS of an assignment statement has un-nested pairs of inner parens, example:
             clone.external_aliases[alias] = (
-                isinstance(table, Join)
-                and table.join_field.related_model._meta.db_table != alias
-            ) or (
-                isinstance(table, BaseTable) and table.table_name != table.table_alias
+                (
+                    isinstance(table, Join)
+                    and table.join_field.related_model._meta.db_table != alias
+                )
+                or (
+                    isinstance(table, BaseTable)
+                    and table.table_name != table.table_alias
+                )
             )
  1. This is bad:
         y_hat_discrete[
             np.arange(y_hat_probas.shape[0]), y_hat_probas.argmax(axis=1)
-        ] = 1
+        ] = (1)

@JelleZijlstra
Copy link
Collaborator

I was going to point out 1. but you beat me :)

I don't think 2. is a problem: the parentheses around each and are preserved, it just adds new parentheses around the whole RHS.

@JelleZijlstra
Copy link
Collaborator

I noticed this one:

-    df["category"] = Series(
-        np.array(list("abcdefghij")).take(np.random.randint(0, 10, size=n))
-    ).astype("category")
+    df["category"] = (
+        Series(
+            np.array(list("abcdefghij")).take(np.random.randint(0, 10, size=n))
+        ).astype("category")
+    )

which I realize is another example of what you meant by 2.

@JelleZijlstra
Copy link
Collaborator

Also wanted to mention that most of the changes in diff-shades are clear improvements. This is going to be a positive change.

@yilei
Copy link
Contributor Author

yilei commented Nov 1, 2022

Adding a note, looks like the handling of trailing comma on the left side is related to an existing issue:

(
    a,
    b,
    c,
    d,
) = func1(arg1)

(a, b, c, d,) = func1(
    arg1
) and func2(arg2)

playground link

Maybe #1671?

@yilei yilei marked this pull request as ready for review November 19, 2022 02:04
@yilei
Copy link
Contributor Author

yilei commented Nov 19, 2022

The three issues mentioned above are now fixed, this is ready to review again.

copybara-service bot pushed a commit to google/pyink that referenced this pull request Dec 6, 2022
This Cherrypicks the changes from psf#3368 before this can be merged upstream.

PiperOrigin-RevId: 493104368
@yilei
Copy link
Contributor Author

yilei commented Dec 15, 2022

Kindly ping? @JelleZijlstra

src/black/linegen.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked over diff-shades again and everything looks good.

rgonzalezfluendo added a commit to fluendo/fluster that referenced this pull request Feb 12, 2024
Adding skip fmt to avoid issues with black < 24.

New rule added in black 24 is:
> If an assignment statement is too long, we now prefer splitting on the right-hand side
> psf/black#3368
rgonzalezfluendo added a commit to fluendo/fluster that referenced this pull request Feb 12, 2024
Adding skip fmt to avoid issues with black < 24.

New rule added in black 24 is:
> If an assignment statement is too long, we now prefer splitting on the right-hand side
> psf/black#3368
mdimopoulos pushed a commit to fluendo/fluster that referenced this pull request Feb 15, 2024
Adding skip fmt to avoid issues with black < 24.

New rule added in black 24 is:
> If an assignment statement is too long, we now prefer splitting on the right-hand side
> psf/black#3368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefer expanding RHS of item assignment over LHS
2 participants