Format subscriptions in a PEP-8 compliant way #178

zsol · 2018-04-29T05:15:20Z

Fixes #157

ambv · 2018-04-29T05:24:07Z

Install pre-commit, fix mypy, rest looks good 👍🏻

zsol · 2018-04-29T05:50:12Z

Looks like we'll need to disable E203

ambv · 2018-04-29T05:52:17Z

E203 is spaces before commas. Where do we need to do that?

zsol · 2018-04-29T05:58:33Z

There's an E203 on this line: https://github.com/ambv/black/blob/1c8a6200c03e8452dde91aeda12f0c6f0738f8dd/black.py#L2104

black.py:2111:56: E203 whitespace before ':'

zsol · 2018-04-29T06:03:04Z

this seems relevant PyCQA/pycodestyle#373

ambv · 2018-04-29T06:04:06Z

Welp, OK, then the rule is stupid. We need to add it to docs as one of the stupid rules (next to W503).

coveralls · 2018-04-29T06:33:51Z

Pull Request Test Coverage Report for Build 310

38 of 38 (100.0%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.02%) to 94.881%

Files with Coverage Reduction	New Missed Lines	%
black.py	1	93.82%

Totals
Change from base Build 297:	0.02%
Covered Lines:	1798
Relevant Lines:	1895

💛 - Coveralls

ambv

Wow, that's a pretty complex patch. Thanks! Blocked on a few things, see comments. Nothing critical but better get it right the first time :-)

ambv · 2018-04-29T06:39:32Z

black.py

+                    subscript_start = child_towards(subscript_start, leaf)
+                complex_subscript = subscript_start is not None and any(
+                    map(lambda n: n.type in EXPRS, subscript_start.pre_order())
+                )


Wouldn't this be shorter as

complex_subscript = subscript_start is not None and any(n.type in EXPRS for n in subscript_start.pre_order())

ambv · 2018-04-29T06:44:27Z

black.py

@@ -726,7 +736,26 @@ def append(self, leaf: Leaf, preformatted: bool = False) -> None:
        if self.leaves and not preformatted:
            # Note: at this point leaf.prefix should be empty except for
            # imports, for which we only preserve newlines.
-            leaf.prefix += whitespace(leaf)
+            lsqb_leaf: Optional[Leaf]


All of this hunting for complex_subscript needs to be its own method. I want append() (and any other method, really) to remain very simple, almost pseudocode.

Call the method is_complex_subscript(self, leaf) and then append() becomes trivial again.

ambv · 2018-04-29T06:48:26Z

black.py

+            if leaf.type == token.LSQB:
+                lsqb_leaf = leaf
+            else:
+                lsqb_leaf = self.bracket_tracker.bracket_match.get(


I consider bracket_match an implementation detail of the bracket_tracker. Create a method like get_open_lsqb() on BracketTracker that we can call directly. Then the code reads less magical.

ambv · 2018-04-29T06:53:16Z

black.py

+            if prevp.type == token.COLON:
+                return NO
+
+            return SPACE if prevp.type == token.COMMA or complex_subscript else NO


When the ternary expression stopped being trivial, it's clearner to just rip out another regular if here. And reword it in a way that only returns NO (see review comment about SPACE below).

ambv · 2018-04-29T06:54:32Z

black.py

@@ -1464,8 +1503,7 @@ def whitespace(leaf: Leaf) -> str:  # noqa C901

            return NO

-        else:
-            return NO
+        return SPACE if complex_subscript else NO


instead:

elif not complex_subscript: return NO

ambv · 2018-04-29T06:55:10Z

black.py

@@ -1349,7 +1388,7 @@ def whitespace(leaf: Leaf) -> str:  # noqa C901

        elif prevp.type == token.COLON:
            if prevp.parent and prevp.parent.type in {syms.subscript, syms.sliceop}:
-                return NO
+                return SPACE if complex_subscript else NO


The default in this function is SPACE. We should avoid returning it from sub-branches.

Instead, just add another check to the if on 1390.

This needs to return SPACE because otherwise control would flow to this block of ifs https://github.com/ambv/black/blob/8f6b5048fcb71bc59399fd7b5511d6e229033b4c/black.py#L1414

Unless you prefer to handle this case down there too

ambv · 2018-04-29T07:11:23Z

black.py

+            complex_subscript = False
+            if lsqb_leaf is not None:
+                subscript_start = lsqb_leaf.next_sibling
+                assert subscript_start is not None, "LSQB always has a right sibling"


This check looks redundant to me. isinstance() below guarantees it's not None for that branch, and subscript_start is not None in complex_subscript = guarantees it's not None later.

ambv · 2018-04-29T07:14:12Z

README.md

+PEP 8 [recommends](https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements)
+to treat ``:`` in slices as a binary operator with the lowest priority, and to
+leave an equal amount of space on either side, except if a parameter is omitted
+(e.g. ``ham[1 + 1:]``). It also states that for extended slices, both ``:``


But in the code we actually have lines like:

self.leaves[_opening_index + 1 :]

So I guess we are always adding a space for complex slices, including when there is just one slice?

Yes, this is a typo. Well spotted :)

ambv · 2018-04-29T07:14:30Z

README.md

@@ -302,6 +302,19 @@ This behaviour may raise ``W503 line break before binary operator`` warnings in
 style guide enforcement tools like Flake8. Since ``W503`` is not PEP 8 compliant,
 you should tell Flake8 to ignore these warnings.

+### Whiespace In Subscripts and ``:``


Just ### Slices would do the trick.

ambv · 2018-04-29T07:22:29Z

black.py

@@ -582,6 +582,16 @@ def show(cls, code: str) -> None:
    syms.listmaker,
    syms.testlist_gexp,
 }
+EXPRS = {


Hm, what you actually wanted here is all children of syms.subscript. If you look at Grammar.txt, you'll see that those are tests. So you're missing quite a few. I won't enumerate all here but try adding this to tests:

slice[lambda: None : lambda: None] slice[1 or 2 : True and False] slice[not so_simple : 1 < val <= 10]

Yeah, those are crazy but valid slices.

- Make `append` trivial again! - move lsqb hunting into `BracketTracker` - Consider expressions complex if they are children of `test` - README section rename and typo fix In two cases I still return `SPACE` explicitly in `whitespace()`. See comments.

ambv

Some last nits to address.

ambv · 2018-04-30T17:47:57Z

black.py

@@ -698,6 +715,13 @@ def maybe_decrement_after_lambda_arguments(self, leaf: Leaf) -> bool:

        return False

+    def get_open_lsqb(self, leaf: Leaf) -> Optional[Leaf]:
+        """Returns the most recent opening square bracket at `leaf` (if any)."""


Adding a leaf argument to get_open_lsqb() creates a rather bizarre signature. It suggests that leaf's position on the line has something to do with the result but it doesn't. It's simply a special case if leaf itself is a LSQB and is not otherwise used. I think that path should be handled directly in is_complex_subscript().

nit: docstrings should be in imperative: "Return" rather than "Returns".

ambv · 2018-04-30T17:48:26Z

black.py

@@ -726,7 +750,9 @@ def append(self, leaf: Leaf, preformatted: bool = False) -> None:
        if self.leaves and not preformatted:
            # Note: at this point leaf.prefix should be empty except for
            # imports, for which we only preserve newlines.
-            leaf.prefix += whitespace(leaf)
+            leaf.prefix += whitespace(


Yeah, much better.

ambv · 2018-04-30T17:52:11Z

black.py

@@ -920,6 +946,22 @@ def remove_trailing_comma(self) -> None:
                self.comments[i] = (comma_index - 1, comment)
        self.leaves.pop()

+    def is_complex_subscript(self, leaf: Leaf) -> bool:
+        """Returns True iff `leaf` is part of a slice with non-trivial exprs."""


Ditto about imperative.

ambv · 2018-04-30T17:52:52Z

black.py

+            elif prevp.type != token.COMMA and not complex_subscript:
+                return NO
+
+            else:


Much better!

nit: else is unnecessary

Format subscriptions in a PEP-8 compliant way

1c8a620

Fixes #157

fix type errors and ignore E203

8f6b504

ambv requested changes Apr 29, 2018

View reviewed changes

Address code review comments:

85ee8b8

- Make `append` trivial again! - move lsqb hunting into `BracketTracker` - Consider expressions complex if they are children of `test` - README section rename and typo fix In two cases I still return `SPACE` explicitly in `whitespace()`. See comments.

ambv reviewed Apr 30, 2018

View reviewed changes

style

ae3b6da

ambv merged commit 9f096d5 into master May 1, 2018

terrelln mentioned this pull request May 7, 2018

Slicing with omitted operand is not formatted according to PEP 8 #194

Closed

ambv mentioned this pull request May 30, 2018

added whitespace in ranged sequence access #279

Closed

zsol deleted the slices branch June 1, 2018 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format subscriptions in a PEP-8 compliant way #178

Format subscriptions in a PEP-8 compliant way #178

zsol commented Apr 29, 2018

ambv commented Apr 29, 2018

zsol commented Apr 29, 2018

ambv commented Apr 29, 2018

zsol commented Apr 29, 2018 •

edited

Loading

zsol commented Apr 29, 2018

ambv commented Apr 29, 2018

coveralls commented Apr 29, 2018 •

edited

Loading

ambv left a comment

ambv Apr 29, 2018

ambv Apr 29, 2018

ambv Apr 29, 2018

ambv Apr 29, 2018

ambv Apr 29, 2018

ambv Apr 29, 2018

zsol Apr 29, 2018

ambv Apr 29, 2018

ambv Apr 29, 2018

zsol Apr 29, 2018

ambv Apr 29, 2018

ambv Apr 29, 2018

ambv left a comment

ambv Apr 30, 2018

ambv Apr 30, 2018

ambv Apr 30, 2018

ambv Apr 30, 2018

Format subscriptions in a PEP-8 compliant way #178

Format subscriptions in a PEP-8 compliant way #178

Conversation

zsol commented Apr 29, 2018

ambv commented Apr 29, 2018

zsol commented Apr 29, 2018

ambv commented Apr 29, 2018

zsol commented Apr 29, 2018 • edited Loading

zsol commented Apr 29, 2018

ambv commented Apr 29, 2018

coveralls commented Apr 29, 2018 • edited Loading

Pull Request Test Coverage Report for Build 310

💛 - Coveralls

ambv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsol commented Apr 29, 2018 •

edited

Loading

coveralls commented Apr 29, 2018 •

edited

Loading