Black: Unnecessary parentheses added when unpacking single item tuples

[jreese]

Describe the bug

Starting with version 19.10b0, black adds unnecessary parentheses around the left side of an assignment when unpacking tuples with a single item. This now produces inconsistent, and IMO less readable, behavior when compared with unpacking multiple items:

(one,) = items
one, two = items
one, two, three = items 

To Reproduce

1. Run black on https://github.com/jreese/aioitertools/blob/master/aioitertools/itertools.py#L340
2. This produces the following diff:

--- a/aioitertools/itertools.py
+++ b/aioitertools/itertools.py
@@ -340,7 +340,7 @@ async def islice(itr: AnyIterable[T], *args: Optional[int]) -> AsyncIterator[T]:
     if not args:
         raise ValueError("must pass stop index")
     if len(args) == 1:
-        stop, = args
+        (stop,) = args
     elif len(args) == 2:
         start, stop = args  # type: ignore
     elif len(args) == 3:

Expected behavior

Previous formatting (without parentheses) should be preserved.

Environment (please complete the following information):

• Version: 19.10b0
• OS and Python version: macOS, cpython 3.6.8 | 3.7.3

Does this bug also happen on master?

Yes

[hugovk]

This is intentional, from https://github.com/psf/black/pull/832.

[jreese]

That is specifically trying to address tuple unpacking that doesn't fit on a single line, and doesn't explain why black prefers (one,) = items but doesn't prefer (one, two) = items.

[zsol]

FWIW I actually like the new behavior, as it's much harder to miss that tuple unpacking when the left hand side is wrapped in parentheses (the same way as they are explicit in one-tuples on the right hand side)

[ambv]

As Zsolt said, this is compatible with single-tuple behavior on the right-hand side. It's an intentional change.

Multi-element tuples are obvious for the reader to see so we are not unnecessarily putting parentheses around them unless that helps with multi-line breaks.

I'll leave this open since other users will likely come to ask about this change, too.

[graingert]

@ambv can you update the docs on this?

[graingert]

Ah to be fair the docs are here

One exception to removing trailing commas is tuple expressions with just one element. In this case Black won't touch the single trailing comma as this would unexpectedly change the underlying data type. Note that this is also the case when commas are used while indexing. This is a tuple in disguise: numpy_array[3, ].

[ambv]

@graingert, you're right that we could put this more front and center.

[Zac-HD]

Here via HypothesisWorks/hypothesis#2157, I expected this to be treated as a bug - mostly because it doesn't appear in the changelog! No actual objection, but documentation would be good 😅

[paranoidi]

after upgrading to 19.10b0 this change was applied

-        abc_item_choices, foobarxy_choices = (
-            self._somesom_account_abc_item_and_foobarxy_choices()
-        )
+        (
+            abc_item_choices,
+            foobarxy_choices,
+        ) = self._somesom_account_abc_item_and_foobarxy_choices()

this does not feel as elegant as it did before ...

[autumnjolitz]

I reverted to 19.3b0 because it actually made the code a lot worse by adding noisy parenthesis and caused lines to go from one to four, which was unacceptably noisy for a codebase that uses tuple unpacking as a fundamental idiom.

[jaraco]

An even simpler example of how black has nitpicked a simple tuple unpacking to the point of absurdity:

def unmatched(pair):
    test_key, item = pair

became

def unmatched(pair):
    test_key, item, = pair

then became

def unmatched(pair):
    (test_key, item,) = pair

and now becomes:

def unmatched(pair):
    (
        test_key,
        item,
    ) = pair

A simple, one-line unpacking, something that's meant to take a pair and unpack it into two variables, now consumes four lines, several unnecessary characters, and is much less readable and less elegant.

I understand the motivations of trying to have consistent formatting across any number of tuple unpacks, but the ultimate effect is to add lint and unnecessary syntax. It's starting to look more like Java or C or lisp and losing the near-pseudocode syntax that I love about Python.

[graingert]

@jaraco I can't repeat this:

graingert@onomastic:~$ cat foo.py
def unmatched(pair):
    test_key, item = pair
graingert@onomastic:~$ pipx run black foo.py
All done! ✨ 🍰 ✨ 'black'...  
1 file left unchanged.
graingert@onomastic:~$ cat foo.py
def unmatched(pair):
    test_key, item = pair

I note that you have two steps, one adding a comma and one splitting over multiple lines. black should never do this because it checks if repeated application makes multiple changes

did you use another tool to add a trailing comma?

[jaraco]

oh! You're right. I may have been mistaken. I traced the log and it seems that the original submission of this code was the second with the trailing comma. I was probably conflating the case of foo, = single_item_list. So glad to see the simple unpacking is supported. Confirmed: jaraco/jaraco.itertools@cc02b11

[graingert]

fwiw, I'd love a flag that strips "magic" trailing commas, # fmt: off and other magics that gives "choice" over formatting. Don't make me think!

[graingert]

@jaraco it's here https://github.com/psf/black/pull/1824