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

consider all optional brackets when splitting #818

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@zsol
Copy link
Collaborator

commented Apr 30, 2019

I'm not married to this solution, I put it together in half an hour. This will likely cause some code churn, but hopefully for the better.

TODO:

  • fix tests
  • verify formatting changes on a (few?) large codebase(s)
  • add changelog
  • better commit message

Fixes #781.

@coveralls

This comment has been minimized.

Copy link

commented Apr 30, 2019

Pull Request Test Coverage Report for Build 996

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 989: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls
@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

I'm running it on our code now. A couple of things I noticed:

-from qcore.asserts import assert_eq, assert_dict_eq, assert_is, assert_unordered_list_eq
+from qcore.asserts import (
+    assert_eq,
+    assert_dict_eq,
+    assert_is,
+    assert_unordered_list_eq,
+)

Here the old line was exactly 88 characters long. I'm guessing we add the parentheses, that pushes it over 88 chars, and then we split it into multiple lines. This issue is causing a lot of churn in our codebase.

-    assert isinstance(value, return_type), (
-        "Value is not in one of the exepected types. Type: %s. Value: %r"
-        % (type(value), value)
+    assert isinstance(
+        value, return_type
+    ), "Value is not in one of the exepected types. Type: %s. Value: %r" % (
+        type(value),
+        value,
     )

Here the old formatting seems better to me.

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

This diff shows more clearly that the optional parens are pushing the line over the limit:

-                    for experiment, settings in exp_settings.future_experiments.items():
+                    for (
+                        experiment,
+                        settings,
+                    ) in exp_settings.future_experiments.items():

The old line length was again exactly 88.

Also overall impact:

1559 files reformatted, 8134 files left unchanged.

@zsol zsol force-pushed the brackets branch from 6b9ff56 to 9889524 May 2, 2019

).push(
# Only send the first n items.
items=items[:num_items]
xxxxxxxxxxxxxxxx = (

This comment has been minimized.

Copy link
@zsol

zsol May 2, 2019

Author Collaborator

I'm not sure about this one. I slightly prefer the old one

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra May 2, 2019

Collaborator

Yes, the extra set of parentheses doesn't really help anything here.

""" % (
_C.__init__.__code__.co_firstlineno + 1,
"""
% (_C.__init__.__code__.co_firstlineno + 1,)

This comment has been minimized.

Copy link
@zsol

zsol May 2, 2019

Author Collaborator

This seems wrong. Haven't figured out what causes this yet

@@ -491,7 +492,7 @@ def gen():
addr_proto,
addr_canonname,
addr_sockaddr,
) in socket.getaddrinfo("google.com", "http"):
) in (socket.getaddrinfo("google.com", "http")):

This comment has been minimized.

Copy link
@zsol

zsol May 2, 2019

Author Collaborator

superfluous parens should be stripped here

@zsol

This comment has been minimized.

Copy link
Collaborator Author

commented May 2, 2019

I updated this PR with another iteration on this idea, which hopefully causes significantly less churn, and gets some pathological cases right. There are still two bugs to figure out though.

black.py Outdated
@@ -1363,6 +1363,8 @@ def __str__(self) -> str:
first = next(leaves)
res = f"{first.prefix}{indent}{first.value}"
for leaf in leaves:
if leaf.type == token.LPAR and not leaf.value and leaf.parent:
maybe_make_parens_invisible_in_atom(leaf.parent)

This comment has been minimized.

Copy link
@zsol

zsol May 2, 2019

Author Collaborator

that's a noop. leaf is already invisible here

"port1": port1_resource,
"port2": port2_resource,
}[port_id]
foo = (

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra May 2, 2019

Collaborator

I think the old code is a lot nicer here.

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

I tried it out again on our codebase. Here's a few notes:

                             assert (
                                 component[0] in VALID_DIRECTIVES
-                            ), "invalid directive %r" % (component,)
+                            ), ("invalid directive %r" % (component,))

These are just useless parens.

-        ] = self.detector.get_busy_time_proportion(None, time_us)
+        ] = (self.detector.get_busy_time_proportion(None, time_us))

Same here.

-        if not set(column_renames.keys()).issubset(columns_to_drop) or not set(
-            column_renames.values()
-        ).issubset(columns_to_add):
+        if (
+            not set(column_renames.keys()).issubset(columns_to_drop)
+            or not set(column_renames.values()).issubset(columns_to_add)
+        ):

This is great.

-            z += h.Button(
-                class_=["ui", "button"],
-                id="clear-search-button",
-                onclick="resetFilter()",
-            )(h.text("Clear"))
+            z += (
+                h.Button(
+                    class_=["ui", "button"],
+                    id="clear-search-button",
+                    onclick="resetFilter()",
+                )(h.text("Clear"))
+            )

Another unnecessary set of parentheses added.

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

Also in terms of churn the effect is similar:
2289 files reformatted, 10702 files left unchanged, 6 files failed to reformat.

The crashes are due to instability. IThe first pass is:

                 "share_data"
-            ] = yield a.app.view.share.mobile.get_native_share_action.asynq(share)
+            ] = (yield a.app.view.share.mobile.get_native_share_action.asynq(share))

And the second is:

-            data[
-                "share_data"
-            ] = (yield a.app.view.share.mobile.get_native_share_action.asynq(share))
+            data["share_data"] = (
+                yield a.app.view.share.mobile.get_native_share_action.asynq(share)
+            )

The eventual result is an improvement though. The other crashes are also cases where the first pass adds parentheses, and the second one rearranges the structure of an assignment + yield.

@zsol zsol force-pushed the brackets branch from 9889524 to a5da1b8 May 6, 2019

@zsol

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2019

This latest one seems promising. I've run it on a large codebase and it did produce a bunch of changes, but all of the changes look better than without this PR.

Most of the changes follow this pattern:

-        if not self.ignore_interrupt and isinstance(
-            sys.exc_info()[1], KeyboardInterrupt
+        if (
+            not self.ignore_interrupt
+            and isinstance(sys.exc_info()[1], KeyboardInterrupt)
         ):

Some asserts have changed as well:

-        assert isinstance(
-            types, collections.Iterable
-        ), "types must be iterable, is %r" % (types,)
+        assert isinstance(types, collections.Iterable), (
+            "types must be iterable, is %r" % (types,)
+        )

The most questionable changes are around manually split strings:

-        sql = "INSERT INTO status_type (type_name, seq)  " "VALUES ('{}', {})".format(
-            key, val
+        sql = (
+            "INSERT INTO status_type (type_name, seq)  "
+            "VALUES ('{}', {})".format(key, val)
         )

All in all, I think this PR makes the formatting better, so ready for review.

@zsol

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2019

As for the amount of churn: on a codebase with half a million LOC across 3k+ files this PR generates

41 files changed, 188 insertions(+), 151 deletions(-)
@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

I ran it on our codebase again. This one is a little questionable:

     def table_string(self):
-        return "%5d  %5d  %5d  %5d  %5d  %5d  %5d  %5d  %5d  %5d" % (
-            self.task_count,
-            self.delete_count,
-            self.suspend_count,
-            self.call_count,
-            self.send_count,
-            self.throw_count,
-            self.return_count,
-            self.cancel_count,
-            self.pause_count,
-            self.resume_count,
+        return (
+            "%5d  %5d  %5d  %5d  %5d  %5d  %5d  %5d  %5d  %5d"
+            % (
+                self.task_count,
+                self.delete_count,
+                self.suspend_count,
+                self.call_count,
+                self.send_count,
+                self.throw_count,
+                self.return_count,
+                self.cancel_count,
+                self.pause_count,
+                self.resume_count,
+            )
         )

On the other hand there are similar cases to this where the tuple after % fits on one line and it looks nicer, so maybe this is the price we have to pay.

This still inserts unnecessary parens:

         assert email_type not in {
             aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
             aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
             aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
-        }, "should not have xxxxxxxxxxxxxxxxxxxxxxx %s" % (email_type)
+        }, ("should not have xxxxxxxxxxxxxxxxxxxxxxx %s" % (email_type))

Overall impact: 604 files reformatted, 12022 files left unchanged.

I agree that most of the changes now are improvements, so I'd be OK with applying it (except for the unnecessary parens in the second diff snippet above).

@zsol

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

I tried hard to remove the unnecessary parens you highlight, but I realized this is a bug that's better addressed in a separate issue. I'm reasonably sure the root cause is the same as with this one:

Black on master does this:

-        assert looooooooooooooooooooong in { looooooooooooooooooooooooooooooooooooooooooooooooooooooong}, "wot. a bug?!" % loooooooong
+        assert looooooooooooooooooooong in {
+            looooooooooooooooooooooooooooooooooooooooooooooooooooooong
+        }, ("wot. a bug?!" % loooooooong)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.