tweak collection literals to explode with trailing comma

[durin42]

This is a change I discussed with @ambv at PyCon - the commit sequence should make things clear. I didn't go to the effort of fixing up existing tests (they'll fail a lot right now), but I can if this looks like a valid approach. This is somewhere between "proof of concept" and something I'd feel happy about, but I wanted feedback on how I'm tackling this early in development since I'm very much noideadog on this code.

[durin42]

If it's of interest, https://phab.mercurial-scm.org/D6342 shows some results on the hg codebase (which is motivating this change).

[JelleZijlstra]

I take it the idea is to explode collection literals on multiple lines when they contain a trailing comma in the input, regardless of their size? I'm not sure I like that idea, because it kind of breaks Black's principle of not depending on the original formatting.

Your diff on hg also seems to show that it's exploding very short tuples unnecessarily: see the two big dicts in mercurial/policy.py.

[JelleZijlstra]

I talked about this some more with @ambv and I'm OK with the change now.

Please fix the CI problems though. If you need help and you're still at PyCon, Lukasz and I are still around, so feel free to find us.

[martinvonz]

Related to @JelleZijlstra's first comment, should we really split up collections within a line like this does in the policy.py example? It would probably be good to add a test case for that either way (i.e. how things like {{1,2},{3,4},} get formatted). Perhaps also test how it gets formatted if some inner collection should get exploded. For example, how should {{1,2,},{3,4}} get formatted? What if the outer collection has a trailing comma? clang-format adds a newline after the trailing comma in all these cases (and doesn't when there is no trailing comma).

[durin42]

I plan on getting back to this tonight. I want to try and stop exploding the sub-containers that lack trailing commas - if anyone has advice for how I should approach that it would be appreciated. I'm not at sprints, but plan to be hacking on this at least some of today and much of Wednesday, and I'm in the same time zone as the sprints in case some sort of real time communication would be helpful.

…

On Mon, May 6, 2019, 11:52 martinvonz ***@***.***> wrote: Related to @JelleZijlstra <https://github.com/JelleZijlstra>'s first comment, should we really split up collections within a line like this does in the policy.py example? It would probably be good to add a test case for that either way (i.e. how things like {{1,2},{3,4},} get formatted). Perhaps also test how it gets formatted if some inner collection should get exploded. For example, how should {{1,2,},{3,4}} get formatted? What if the outer collection has a trailing comma? clang-format adds a newline after the trailing comma in all these cases (and doesn't when there is no trailing comma). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#826 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAE6LKUX2W2LKNNMEWUPZDPUBH4XANCNFSM4HK2GOOQ> .

[durin42]

Please take a look - I think this is workable now.

I can't say I'm 100% thrilled with my approach in some corners, but I've also got no idea what I'm doing. :)

https://phab.mercurial-scm.org/D6342 is updated with the new results from this patch.

[JelleZijlstra]

I ran your branch on my codebase and noticed some changes like this:

-    DEFAULT_DIRS = (str(AIRFLOW_DIR / "lib/floq"),)
+    DEFAULT_DIRS = (
+        str(AIRFLOW_DIR / "lib/floq"),
+    )

This seems undesirable; we need the trailing comma to make the tuple, but that doesn't mean we need to explode it.

[JelleZijlstra]

Another example where it's especially ugly:

-                pieces.append("awaiting %r" % (coro.cr_await,))
+                pieces.append(
+                    "awaiting %r"
+                    % (
+                        coro.cr_await,
+                    )
+                )

[durin42]

Huh. I guess my defense against one-element tuples isn't good enough. I'll try and figure it out, but if @ambv has any ideas I'd love to hear them. :/

[ambv]

Unfortunately no genius suggestions here but I agree we need to not explode them.

[durin42]

Oh, I see. I explicitly detected the case of [LPAR, one_token, COMMA, RPAR], so I'll miss any one-tuple that's got a more complicated construct inside. :(

Is there any prior art for matching a one-tuple in the codebase?

[ambv]

Use black.is_one_tuple().

[durin42]

I'm a little lost - that takes a Node or Leaf, but I have a Line in the place where I'm doing the detection (look at is_collection_with_optional_trailing_comma) - it doesn't look like is_one_tuple quite fits? Or am I missing something obvious?

[ambv]

You're right. Feeding leaves to a Line is flattening the structures so you cannot use is_one_tuple() anymore. Before I was handling this in lines 1303 - 1324 that you removed. Maybe you can reuse that code?

[durin42]

That code was helpful, moved it to another property on Line because it let the code read more clearly.

See what you think? I added test cases based on the misformattings upthread.

[JelleZijlstra]

I'm still seeing some issues in my codebase:

-        "sort_key": ("pid",),
-        "dist_key": ("pid",),
+        "sort_key": (
+            "pid",
+        ),
+        "dist_key": (
+            "pid",
+        ),

             yield create_alert(
                 "later_updates_not_blank",
-                "incorrect updates returned after message sent: %r" % (updates,),
+                "incorrect updates returned after message sent: %r"
+                % (
+                    updates,
+                ),
             )

-                ["git", "clone", "https://github.com/quora/%s.git" % (external_name,)]
+                [
+                    "git",
+                    "clone",
+                    "https://github.com/quora/%s.git"
+                    % (
+                        external_name,
+                    ),
+                ]

Seems like it still explodes one-tuples within other collections.

[JelleZijlstra]

What caused this change? Black should put a trailing comma here.

[durin42]

You're right, but you're commenting on the pre-formatted part of the test case, not the result. The spirit of the test appeared to be that the comment moved correctly with the collapsed line, so I removed the trailing comma in the input.

[JelleZijlstra]

Thanks! Sorry for that, it's not always easy to tell from changes in the test suite what is actually changing.

[durin42]

Addressed those cases too. I'm off to bed now (got very nerdsniped by this one-tuple problem), but the tests I constructed based on your examples pass. I'm decreasingly happy with the code in is_trailing_comma_because_of_one_tuple, so maybe take a look at that if you have time and see if there's something cleaner I could do? Detecting the one-tuple case is harder than I expected.

[JelleZijlstra]

The current version crashes on this file:

import core, time, a

i = 20000

The stack trace ends:

  File "/home/jelle/repos/black/black.py", line 821, in visit
    yield from getattr(self, f"visit_{name}", self.visit_default)(node)
  File "/home/jelle/repos/black/black.py", line 1602, in visit_default
    self.current_line.append(node)
  File "/home/jelle/repos/black/black.py", line 1144, in append
    self.maybe_remove_trailing_comma(leaf)
  File "/home/jelle/repos/black/black.py", line 1355, in maybe_remove_trailing_comma
    or (self.leaves[-6].value == "import" and self.leaves[-3].value == "as")
IndexError: list index out of range

It also caused a bunch of changes to my (already formatted) codebase, which is unexpected. It's hard to tell for me right now exactly what the changes are because I messed up distinguishing this diff from the other changes in Black master (I'll redo the formatting later and try to distinguish the changes properly), but I think it caused this ugly change:

-                    assert False, (
-                        "could not insert even though method found: \n%s"
-                        % "".join(ret[-10:])
+                    assert (
+                        False
+                    ), "could not insert even though method found: \n%s" % "".join(
+                        ret[-10:]
                     )

[durin42]

Alrighty, my discomfort with how I was handling oneples wore me down. I have reworked the optional-trailing-comma detection pretty extensively, and it feels better to me. At this point, the "meat" of the change is all in the is_collection_with_optional_trailing_comma property.

@JelleZijlstra I'm curious if this works on your codebase now. You're right that you shouldn't see changes on already-blackened code. Thanks for all the test cases - it's helped a lot.

[JelleZijlstra]

I tried it again and got a similar set of changes, like this:

-    IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING = (
-        Config.IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING
-        | {pylons.controllers.WSGIController}
-    )
+    IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING = Config.IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING | {
+        pylons.controllers.WSGIController
+    }

It also still crashes on the file with import core, time, a; am I using the wrong commit? This was running commit f1fb54003469ae846e4d9d2ff137c546d648e35f.

[durin42]

Sigh, the import thing. That was a latent bug, now tested and fixed.

As for the other issue: the line in your left hand diff side gets reformatted by black for me without my patches applied. Check again? I even added that exact case as a test to collections.py and you can see it gets reformatted even in the base commit of this chain, where I haven't changed any black behavior yet.

[benjamin-kirkbride]

@JelleZijlstra Can you please explain what changed your opinion on this matter?

One of the most appealing things about Black is that (with few exceptions?) the only thing that affects code formatting is the AST and line length. This obviously goes against that.

[thinker3]

I ran your branch on my codebase and noticed some changes like this:

-    DEFAULT_DIRS = (str(AIRFLOW_DIR / "lib/floq"),)
+    DEFAULT_DIRS = (
+        str(AIRFLOW_DIR / "lib/floq"),
+    )

This seems undesirable; we need the trailing comma to make the tuple, but that doesn't mean we need to explode it.

need an custom option whether to explode one-element tuple