Reconsider formatting that was changed by #762

[JelleZijlstra]

#762 fixed broken code, but in many cases the old, broken code produced subjectively better formatting, especially (in my experience) around assert statements.

Let's consider whether there's a way to improve the formatting in cases that were affected by #762.

It would be helpful to add examples to this issue of formatting changes caused by #762 that may not be improvements.

[rouge8]

I think this might also be a bug, because it's now producing some lines that are longer than 88 characters. 😞

An examples from one of our codebases:

Before

class Foo:

    def meth(self):
        allow_partial_registration = (
            self.allow_partial_registration_completion
            and state.get("allow_partial_registration")
        )

After

class Foo:

    def meth(self):
        # the next line is 94 characters!
        allow_partial_registration = self.allow_partial_registration_completion and state.get(
            "allow_partial_registration"
        )

I also see it with several tests, where #762 changed

assert (
    something_long
    == something_else_very_long.method_call(x)
)

into

# in the actual code base, these exceed 88 characters
assert something_long == something_else_very_long.method_call(
    x
)

[zsol]

I suspect most of these problems stem from the fact that black doesn't consider all possible parenthesis while doing right_hand_split (invisible parens don't count).

I'm toying with a change that converts all invisible parenthesis to visible before doing the formatting, but haven't yet figured out how to eliminate "useless" ones afterwards yet.

[ambv]

It would be a simpler change to change if bt.delimiter_count_with_priority(max_priority) > 1 to be effectively ">= 1". I considered it at the time and I was on the fence on whether I liked the additional parentheses (which often cause more vertical space to be used).

[zsol]

I agree that would result in simpler code in Black, but in cases where there's only one delimiter with max priority and it's towards the end of the line, it ends up being subjectively worse in my opinion. Case in point:

-        return (first_leaf.type == token.NAME and first_leaf.value == "def") or (
-            first_leaf.type == token.ASYNC
-            and second_leaf is not None
-            and second_leaf.type == token.NAME
-            and second_leaf.value == "def"
+        return (
+            (first_leaf.type == token.NAME and first_leaf.value == "def")
+            or (
+                first_leaf.type == token.ASYNC
+                and second_leaf is not None
+                and second_leaf.type == token.NAME
+                and second_leaf.value == "def"
+            )
         )

or a more extreme example:

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

[zsol]

On the other hand, your suggestion would make the examples in #348 look better. I'll try that version internally and see the effect.

[zsol]

These are some other terrible examples:

-        return (
-            os.path.relpath(os.path.realpath(path), "somedir")
-            .replace(".pyc", "")
-            .replace(".py", "")
-        )
+        return os.path.relpath(os.path.realpath(path), "somedir").replace(
+            ".pyc", ""
+        ).replace(".py", "")

It breaks some fluent interfaces:

-        version = (
-            await NodePhabricatorVersion.query_all()
-            .where_phabricator_diff_fbid(P.equals(diff.id))
-            .order_by_created_time_asc()
-            .first()
-        )
+        version = await NodePhabricatorVersion.query_all().where_phabricator_diff_fbid(
+            P.equals(diff.id)
+        ).order_by_created_time_asc().first()

These are all probably fixable by sprinkling some more heuristics in can_omit_invisible_parens, but at this point we're not better off than #818, so I'm going to go for that approach.

[gr4viton]

I suspect most of these problems stem from the fact that black doesn't consider all possible parenthesis while doing right_hand_split (invisible parens don't count).

I'm toying with a change that converts all invisible parenthesis to visible before doing the formatting, but haven't yet figured out how to eliminate "useless" ones afterwards yet.
#781 (comment)

Possible way to eliminate "useless" ones afterwards.

I do not know how black annotates individual characters - if it annotates them at all.
If every char is not only "an index in the str", but is a separate object. Then you can do what you want without the need to eliminate the the "invisibal parenthesis" afterwards

Eg. str would get converted to AnnotatedStr where in annotated you can store any info for each string

txt = "if a == b and c == d:"
astr = AnnotatedStr(txt)
assert astr.text_raw == txt
astr.show_annotated == "if (a == b) and (c == d):"

Then to get the text_raw formatted as with anotations, you would go alphanum chars through the annotated and formated text and would only print those chars which are in the original.

formatted = "".join(
   ch for ch in asrt.show_annotated if ch == " " or ch in asrt.text_raw
)