From cb5aadad74c0a1c9c514a633c632c99b668c70ed Mon Sep 17 00:00:00 2001 From: =?utf8?q?=C5=81ukasz=20Langa?= Date: Wed, 4 Apr 2018 21:45:01 -0700 Subject: [PATCH] Automatic parentheses management Fixes #4 --- README.md | 18 ++ black.py | 251 ++++++++++++++++++++----- docs/reference/reference_functions.rst | 10 + tests/composition.py | 13 ++ tests/empty_lines.py | 51 ++--- tests/expression.diff | 22 ++- tests/expression.py | 19 +- tests/function.py | 4 +- tests/import_spacing.py | 10 +- tests/test_black.py | 9 +- 10 files changed, 336 insertions(+), 71 deletions(-) diff --git a/README.md b/README.md index 35f1379..06b7324 100644 --- a/README.md +++ b/README.md @@ -300,6 +300,22 @@ 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. +### Parentheses + +Some parentheses are optional in the Python grammar. Any expression can +be wrapped in a pair of parentheses to form an atom. There are a few +interesting cases: + +- `if (...):` +- `while (...):` +- `for (...) in (...):` +- `assert (...), (...)` +- `from X import (...)` + +In those cases, parentheses are removed when the entire statement fits +in one line, or if the inner expression doesn't have any delimiters to +further split on. Otherwise, the parentheses are always added. + ## Editor integration @@ -479,6 +495,8 @@ More details can be found in [CONTRIBUTING](CONTRIBUTING.md). * added `--quiet` (#78) +* added automatic parentheses management (#4) + * added [pre-commit](https://pre-commit.com) integration (#103, #104) * fixed reporting on `--check` with multiple files (#101, #102) diff --git a/black.py b/black.py index 19871aa..3e3ae0a 100644 --- a/black.py +++ b/black.py @@ -17,6 +17,7 @@ import sys from typing import ( Any, Callable, + Collection, Dict, Generic, Iterable, @@ -315,12 +316,12 @@ def format_stdin_to_stdout( `line_length` and `fast` arguments are passed to :func:`format_file_contents`. """ src = sys.stdin.read() + dst = src try: dst = format_file_contents(src, line_length=line_length, fast=fast) return True except NothingChanged: - dst = src return False finally: @@ -582,6 +583,7 @@ class BracketTracker: """Return the highest priority of a delimiter found on the line. Values are consistent with what `is_delimiter()` returns. + Raises ValueError on no delimiters. """ return max(v for k, v in self.delimiters.items() if k not in exclude) @@ -608,7 +610,7 @@ class Line: Inline comments are put aside. """ - has_value = leaf.value.strip() + has_value = leaf.type in BRACKETS or bool(leaf.value.strip()) if not has_value: return @@ -709,12 +711,12 @@ class Line: and self.leaves[0].value == "yield" ) - @property - def contains_standalone_comments(self) -> bool: + def contains_standalone_comments(self, depth_limit: int = sys.maxsize) -> bool: """If so, needs to be split before emitting.""" for leaf in self.leaves: if leaf.type == STANDALONE_COMMENT: - return True + if leaf.bracket_depth <= depth_limit: + return True return False @@ -1077,15 +1079,22 @@ class LineGenerator(Visitor[Line]): # DEDENT has no value. Additionally, in blib2to3 it never holds comments. yield from self.line(-1) - def visit_stmt(self, node: Node, keywords: Set[str]) -> Iterator[Line]: + def visit_stmt( + self, node: Node, keywords: Set[str], parens: Set[str] + ) -> Iterator[Line]: """Visit a statement. This implementation is shared for `if`, `while`, `for`, `try`, `except`, - `def`, `with`, and `class`. + `def`, `with`, `class`, and `assert`. + + The relevant Python language `keywords` for a given statement will be + NAME leaves within it. This methods puts those on a separate line. - The relevant Python language `keywords` for a given statement will be NAME - leaves within it. This methods puts those on a separate line. + `parens` holds pairs of nodes where invisible parentheses should be put. + Keys hold nodes after which opening parentheses should be put, values + hold nodes before which closing parentheses should be put. """ + normalize_invisible_parens(node, parens_after=parens) for child in node.children: if child.type == token.NAME and child.value in keywords: # type: ignore yield from self.line() @@ -1125,6 +1134,32 @@ class LineGenerator(Visitor[Line]): yield from self.line() yield from self.visit(child) + def visit_import_from(self, node: Node) -> Iterator[Line]: + """Visit import_from and maybe put invisible parentheses. + + This is separate from `visit_stmt` because import statements don't + support arbitrary atoms and thus handling of parentheses is custom. + """ + check_lpar = False + for index, child in enumerate(node.children): + if check_lpar: + if child.type == token.LPAR: + # make parentheses invisible + child.value = "" # type: ignore + node.children[-1].value = "" # type: ignore + else: + # insert invisible parentheses + node.insert_child(index, Leaf(token.LPAR, "")) + node.append_child(Leaf(token.RPAR, "")) + break + + check_lpar = ( + child.type == token.NAME and child.value == "import" # type: ignore + ) + + for child in node.children: + yield from self.visit(child) + def visit_SEMI(self, leaf: Leaf) -> Iterator[Line]: """Remove a semicolon and put the other statement on a separate line.""" yield from self.line() @@ -1155,18 +1190,23 @@ class LineGenerator(Visitor[Line]): def __attrs_post_init__(self) -> None: """You are in a twisty little maze of passages.""" v = self.visit_stmt - self.visit_if_stmt = partial(v, keywords={"if", "else", "elif"}) - self.visit_while_stmt = partial(v, keywords={"while", "else"}) - self.visit_for_stmt = partial(v, keywords={"for", "else"}) - self.visit_try_stmt = partial(v, keywords={"try", "except", "else", "finally"}) - self.visit_except_clause = partial(v, keywords={"except"}) - self.visit_funcdef = partial(v, keywords={"def"}) - self.visit_with_stmt = partial(v, keywords={"with"}) - self.visit_classdef = partial(v, keywords={"class"}) + Ø: Set[str] = set() + self.visit_assert_stmt = partial(v, keywords={"assert"}, parens={"assert", ","}) + self.visit_if_stmt = partial(v, keywords={"if", "else", "elif"}, parens={"if"}) + self.visit_while_stmt = partial(v, keywords={"while", "else"}, parens={"while"}) + self.visit_for_stmt = partial(v, keywords={"for", "else"}, parens={"for", "in"}) + self.visit_try_stmt = partial( + v, keywords={"try", "except", "else", "finally"}, parens=Ø + ) + self.visit_except_clause = partial(v, keywords={"except"}, parens=Ø) + self.visit_with_stmt = partial(v, keywords={"with"}, parens=Ø) + self.visit_funcdef = partial(v, keywords={"def"}, parens=Ø) + self.visit_classdef = partial(v, keywords={"class"}, parens=Ø) self.visit_async_funcdef = self.visit_async_stmt self.visit_decorated = self.visit_decorators +IMPLICIT_TUPLE = {syms.testlist, syms.testlist_star_expr, syms.exprlist} BRACKET = {token.LPAR: token.RPAR, token.LSQB: token.RSQB, token.LBRACE: token.RBRACE} OPENING_BRACKETS = set(BRACKET.keys()) CLOSING_BRACKETS = set(BRACKET.values()) @@ -1215,14 +1255,17 @@ def whitespace(leaf: Leaf) -> str: # noqa C901 return prevp.prefix elif prevp.type == token.DOUBLESTAR: - if prevp.parent and prevp.parent.type in { - syms.arglist, - syms.argument, - syms.dictsetmaker, - syms.parameters, - syms.typedargslist, - syms.varargslist, - }: + if ( + prevp.parent + and prevp.parent.type in { + syms.arglist, + syms.argument, + syms.dictsetmaker, + syms.parameters, + syms.typedargslist, + syms.varargslist, + } + ): return NO elif prevp.type == token.COLON: @@ -1382,9 +1425,10 @@ def whitespace(leaf: Leaf) -> str: # noqa C901 prevp_parent = prevp.parent assert prevp_parent is not None - if prevp.type == token.COLON and prevp_parent.type in { - syms.subscript, syms.sliceop - }: + if ( + prevp.type == token.COLON + and prevp_parent.type in {syms.subscript, syms.sliceop} + ): return NO elif prevp.type == token.EQUAL and prevp_parent.type == syms.argument: @@ -1607,7 +1651,7 @@ def split_line( if ( len(line_str) <= line_length and "\n" not in line_str # multiline strings - and not line.contains_standalone_comments + and not line.contains_standalone_comments() ): yield line return @@ -1673,9 +1717,7 @@ def left_hand_split(line: Line, py36: bool = False) -> Iterator[Line]: if body_leaves: normalize_prefix(body_leaves[0], inside_brackets=True) # Build the new lines. - for result, leaves in ( - (head, head_leaves), (body, body_leaves), (tail, tail_leaves) - ): + for result, leaves in (head, head_leaves), (body, body_leaves), (tail, tail_leaves): for leaf in leaves: result.append(leaf, preformatted=True) for comment_after in line.comments_after(leaf): @@ -1686,7 +1728,9 @@ def left_hand_split(line: Line, py36: bool = False) -> Iterator[Line]: yield result -def right_hand_split(line: Line, py36: bool = False) -> Iterator[Line]: +def right_hand_split( + line: Line, py36: bool = False, omit: Collection[LeafID] = () +) -> Iterator[Line]: """Split line into many lines, starting with the last matching bracket pair.""" head = Line(depth=line.depth) body = Line(depth=line.depth + 1, inside_brackets=True) @@ -1696,14 +1740,16 @@ def right_hand_split(line: Line, py36: bool = False) -> Iterator[Line]: head_leaves: List[Leaf] = [] current_leaves = tail_leaves opening_bracket = None + closing_bracket = None for leaf in reversed(line.leaves): if current_leaves is body_leaves: if leaf is opening_bracket: current_leaves = head_leaves if body_leaves else tail_leaves current_leaves.append(leaf) if current_leaves is tail_leaves: - if leaf.type in CLOSING_BRACKETS: + if leaf.type in CLOSING_BRACKETS and id(leaf) not in omit: opening_bracket = leaf.opening_bracket + closing_bracket = leaf current_leaves = body_leaves tail_leaves.reverse() body_leaves.reverse() @@ -1711,15 +1757,36 @@ def right_hand_split(line: Line, py36: bool = False) -> Iterator[Line]: # Since body is a new indent level, remove spurious leading whitespace. if body_leaves: normalize_prefix(body_leaves[0], inside_brackets=True) + elif not head_leaves: + # No `head` and no `body` means the split failed. `tail` has all content. + raise CannotSplit("No brackets found") + # Build the new lines. - for result, leaves in ( - (head, head_leaves), (body, body_leaves), (tail, tail_leaves) - ): + for result, leaves in (head, head_leaves), (body, body_leaves), (tail, tail_leaves): for leaf in leaves: result.append(leaf, preformatted=True) for comment_after in line.comments_after(leaf): result.append(comment_after, preformatted=True) bracket_split_succeeded_or_raise(head, body, tail) + assert opening_bracket and closing_bracket + if ( + opening_bracket.type == token.LPAR + and not opening_bracket.value + and closing_bracket.type == token.RPAR + and not closing_bracket.value + ): + # These parens were optional. If there aren't any delimiters or standalone + # comments in the body, they were unnecessary and another split without + # them should be attempted. + if not ( + body.bracket_tracker.delimiters or line.contains_standalone_comments(0) + ): + omit = {id(closing_bracket), *omit} + yield from right_hand_split(line, py36=py36, omit=omit) + return + + ensure_visible(opening_bracket) + ensure_visible(closing_bracket) for result in (head, body, tail): if result: yield result @@ -1833,12 +1900,7 @@ def delimiter_split(line: Line, py36: bool = False) -> Iterator[Line]: @dont_increase_indentation def standalone_comment_split(line: Line, py36: bool = False) -> Iterator[Line]: """Split standalone comments from the rest of the line.""" - for leaf in line.leaves: - if leaf.type == STANDALONE_COMMENT: - if leaf.bracket_depth == 0: - break - - else: + if not line.contains_standalone_comments(0): raise CannotSplit("Line does not have any standalone comments") current_line = Line(depth=line.depth, inside_brackets=line.inside_brackets) @@ -1950,6 +2012,109 @@ def normalize_string_quotes(leaf: Leaf) -> None: leaf.value = f"{prefix}{new_quote}{new_body}{new_quote}" +def normalize_invisible_parens(node: Node, parens_after: Set[str]) -> None: + """Make existing optional parentheses invisible or create new ones. + + Standardizes on visible parentheses for single-element tuples, and keeps + existing visible parentheses for other tuples and generator expressions. + """ + check_lpar = False + for child in list(node.children): + if check_lpar: + if child.type == syms.atom: + if not ( + is_empty_tuple(child) + or is_one_tuple(child) + or max_delimiter_priority_in_atom(child) >= COMMA_PRIORITY + ): + first = child.children[0] + last = child.children[-1] + if first.type == token.LPAR and last.type == token.RPAR: + # make parentheses invisible + first.value = "" # type: ignore + last.value = "" # type: ignore + elif is_one_tuple(child): + # wrap child in visible parentheses + lpar = Leaf(token.LPAR, "(") + rpar = Leaf(token.RPAR, ")") + index = child.remove() or 0 + node.insert_child(index, Node(syms.atom, [lpar, child, rpar])) + else: + # wrap child in invisible parentheses + lpar = Leaf(token.LPAR, "") + rpar = Leaf(token.RPAR, "") + index = child.remove() or 0 + node.insert_child(index, Node(syms.atom, [lpar, child, rpar])) + + check_lpar = isinstance(child, Leaf) and child.value in parens_after + + +def is_empty_tuple(node: LN) -> bool: + """Return True if `node` holds an empty tuple.""" + return ( + node.type == syms.atom + and len(node.children) == 2 + and node.children[0].type == token.LPAR + and node.children[1].type == token.RPAR + ) + + +def is_one_tuple(node: LN) -> bool: + """Return True if `node` holds a tuple with one element, with or without parens.""" + if node.type == syms.atom: + if len(node.children) != 3: + return False + + lpar, gexp, rpar = node.children + if not ( + lpar.type == token.LPAR + and gexp.type == syms.testlist_gexp + and rpar.type == token.RPAR + ): + return False + + return len(gexp.children) == 2 and gexp.children[1].type == token.COMMA + + return ( + node.type in IMPLICIT_TUPLE + and len(node.children) == 2 + and node.children[1].type == token.COMMA + ) + + +def max_delimiter_priority_in_atom(node: LN) -> int: + if node.type != syms.atom: + return 0 + + first = node.children[0] + last = node.children[-1] + if first.type == token.LPAR and last.type == token.RPAR: + bt = BracketTracker() + for c in node.children[1:-1]: + if isinstance(c, Leaf): + bt.mark(c) + else: + for leaf in c.leaves(): + bt.mark(leaf) + try: + return bt.max_delimiter_priority() + + except ValueError: + return 0 + + +def ensure_visible(leaf: Leaf) -> None: + """Make sure parentheses are visible. + + They could be invisible as part of some statements (see + :func:`normalize_invible_parens` and :func:`visit_import_from`). + """ + if leaf.type == token.LPAR: + leaf.value = "(" + elif leaf.type == token.RPAR: + leaf.value = ")" + + def is_python36(node: Node) -> bool: """Return True if the current file is using Python 3.6+ features. diff --git a/docs/reference/reference_functions.rst b/docs/reference/reference_functions.rst index 4e7fb83..2b8c08e 100644 --- a/docs/reference/reference_functions.rst +++ b/docs/reference/reference_functions.rst @@ -18,8 +18,12 @@ Assertions and checks .. autofunction:: black.is_delimiter +.. autofunction:: black.is_empty_tuple + .. autofunction:: black.is_import +.. autofunction:: black.is_one_tuple + .. autofunction:: black.is_python36 Formatting @@ -58,6 +62,8 @@ Split functions .. autofunction:: black.right_hand_split +.. autofunction:: black.standalone_comment_split + .. autofunction:: black.split_line .. autofunction:: black.bracket_split_succeeded_or_raise @@ -71,6 +77,8 @@ Utilities .. autofunction:: black.diff +.. autofunction:: black.ensure_visible + .. autofunction:: black.generate_comments .. autofunction:: black.make_comment @@ -79,6 +87,8 @@ Utilities .. autofunction:: black.normalize_string_quotes +.. autofunction:: black.normalize_invisible_parens + .. autofunction:: black.preceding_leaf .. autofunction:: black.whitespace diff --git a/tests/composition.py b/tests/composition.py index fb27b3e..287888d 100644 --- a/tests/composition.py +++ b/tests/composition.py @@ -19,3 +19,16 @@ class C: "2 files reformatted, 2 files left unchanged, " "2 files failed to reformat.", ) + for i in (a,): + if ( + # Rule 1 + i % 2 == 0 + # Rule 2 + and i % 3 == 0 + ): + while ( + # Just a comment + call() + # Another + ): + print(i) diff --git a/tests/empty_lines.py b/tests/empty_lines.py index d001db4..0edeb01 100644 --- a/tests/empty_lines.py +++ b/tests/empty_lines.py @@ -113,23 +113,29 @@ def f(): return NO if prevp.type == token.EQUAL: - if prevp.parent and prevp.parent.type in { - syms.typedargslist, - syms.varargslist, - syms.parameters, - syms.arglist, - syms.argument, - }: + if ( + prevp.parent + and prevp.parent.type in { + syms.typedargslist, + syms.varargslist, + syms.parameters, + syms.arglist, + syms.argument, + } + ): return NO elif prevp.type == token.DOUBLESTAR: - if prevp.parent and prevp.parent.type in { - syms.typedargslist, - syms.varargslist, - syms.parameters, - syms.arglist, - syms.dictsetmaker, - }: + if ( + prevp.parent + and prevp.parent.type in { + syms.typedargslist, + syms.varargslist, + syms.parameters, + syms.arglist, + syms.dictsetmaker, + } + ): return NO @@ -167,11 +173,14 @@ def g(): return NO if prevp.type == token.EQUAL: - if prevp.parent and prevp.parent.type in { - syms.typedargslist, - syms.varargslist, - syms.parameters, - syms.arglist, - syms.argument, - }: + if ( + prevp.parent + and prevp.parent.type in { + syms.typedargslist, + syms.varargslist, + syms.parameters, + syms.arglist, + syms.argument, + } + ): return NO diff --git a/tests/expression.diff b/tests/expression.diff index f37b16b..965aa00 100644 --- a/tests/expression.diff +++ b/tests/expression.diff @@ -103,7 +103,7 @@ ] slice[0] slice[0:1] -@@ -114,73 +123,92 @@ +@@ -114,78 +123,104 @@ numpy[-(c + 1):, d] numpy[:, l[-2]] numpy[:, ::-1] @@ -154,7 +154,12 @@ async def f(): await some.complicated[0].call(with_args=(True or (1 is not 1))) - +-for x, in (1,), (2,), (3,): ... +-for y in (): ... +-for z in (i for i in (1, 2, 3)): ... +-for i in (call()): ... +-for j in (1 + (2 + 3)): ... +-while(this and that): ... -if ( - threading.current_thread() != threading.main_thread() and - threading.current_thread() != threading.main_thread() or @@ -192,6 +197,19 @@ -): - return True + ++ ++for (x,) in (1,), (2,), (3,): ++ ... ++for y in (): ++ ... ++for z in (i for i in (1, 2, 3)): ++ ... ++for i in call(): ++ ... ++for j in 1 + (2 + 3): ++ ... ++while this and that: ++ ... +if ( + threading.current_thread() != threading.main_thread() + and threading.current_thread() != threading.main_thread() diff --git a/tests/expression.py b/tests/expression.py index 3cd0c61..b03abfa 100644 --- a/tests/expression.py +++ b/tests/expression.py @@ -144,7 +144,12 @@ def gen(): async def f(): await some.complicated[0].call(with_args=(True or (1 is not 1))) - +for x, in (1,), (2,), (3,): ... +for y in (): ... +for z in (i for i in (1, 2, 3)): ... +for i in (call()): ... +for j in (1 + (2 + 3)): ... +while(this and that): ... if ( threading.current_thread() != threading.main_thread() and threading.current_thread() != threading.main_thread() or @@ -356,6 +361,18 @@ async def f(): await some.complicated[0].call(with_args=(True or (1 is not 1))) +for (x,) in (1,), (2,), (3,): + ... +for y in (): + ... +for z in (i for i in (1, 2, 3)): + ... +for i in call(): + ... +for j in 1 + (2 + 3): + ... +while this and that: + ... if ( threading.current_thread() != threading.main_thread() and threading.current_thread() != threading.main_thread() diff --git a/tests/function.py b/tests/function.py index 584474c..6cf6dbc 100644 --- a/tests/function.py +++ b/tests/function.py @@ -33,7 +33,7 @@ def spaces(a=1, b=(), c=[], d={}, e=True, f=-1, g=1 if False else 2, h="", i=r'' assert task._cancel_stack[:len(old_stack)] == old_stack def spaces_types(a: int = 1, b: tuple = (), c: list = [], d: dict = {}, e: bool = True, f: int = -1, g: int = 1 if False else 2, h: str = "", i: str = r''): ... def spaces2(result= _core.Value(None)): - ... + assert fut is self._read_fut, (fut, self._read_fut) # EMPTY LINE WITH WHITESPACE (this comment will be removed) def example(session): result = session.query(models.Customer.id).filter( @@ -146,7 +146,7 @@ def spaces_types( def spaces2(result=_core.Value(None)): - ... + assert fut is self._read_fut, (fut, self._read_fut) def example(session): diff --git a/tests/import_spacing.py b/tests/import_spacing.py index e9f7330..4091148 100644 --- a/tests/import_spacing.py +++ b/tests/import_spacing.py @@ -18,7 +18,10 @@ from ..queues import * from ..streams import * from .a.b.c.subprocess import * -from . import tasks +from . import (tasks) +from . import (A, B, C) +from . import SomeVeryLongNameAndAllOfItsAdditionalLetters1, \ + SomeVeryLongNameAndAllOfItsAdditionalLetters2 __all__ = ( base_events.__all__ @@ -58,6 +61,11 @@ from ..streams import * from .a.b.c.subprocess import * from . import tasks +from . import A, B, C +from . import ( + SomeVeryLongNameAndAllOfItsAdditionalLetters1, + SomeVeryLongNameAndAllOfItsAdditionalLetters2, +) __all__ = ( base_events.__all__ diff --git a/tests/test_black.py b/tests/test_black.py index d2359cb..f71f9b3 100644 --- a/tests/test_black.py +++ b/tests/test_black.py @@ -174,7 +174,14 @@ class BlackTestCase(unittest.TestCase): sys.stdout = hold_stdout os.unlink(tmp_file) actual = actual.rstrip() + "\n" # the diff output has a trailing space - self.assertEqual(expected, actual) + if expected != actual: + dump = black.dump_to_file(actual) + msg = ( + f"Expected diff isn't equal to the actual. If you made changes " + f"to expression.py and this is an anticipated difference, " + f"overwrite tests/expression.diff with {dump}." + ) + self.assertEqual(expected, actual, msg) @patch("black.dump_to_file", dump_to_stderr) def test_fstring(self) -> None: -- 2.39.5