-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Remove unnecessary parentheses from with
statements
#2926
Changes from 16 commits
1a499b2
1a2c13b
27a4bfd
ddc1a3f
240a6f8
cd0695d
0e0d9f8
3c14ab2
254fc91
d5db697
87aa74d
aaa902a
9b62d0e
68bb6d8
3f8db3d
ac698f8
6532485
e61783a
639a2e4
afcb8be
1540bfa
0f0a01a
bd7373c
99167c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -319,7 +319,10 @@ def __post_init__(self) -> None: | |
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=Ø) | ||
if self.mode.preview: | ||
self.visit_with_stmt = partial(v, keywords={"with"}, parens={"with"}) | ||
else: | ||
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_expr_stmt = partial(v, keywords=Ø, parens=ASSIGNMENTS) | ||
|
@@ -840,11 +843,26 @@ def normalize_invisible_parens( | |
check_lpar = True | ||
|
||
if check_lpar: | ||
if child.type == syms.atom: | ||
if ( | ||
preview | ||
and child.type == syms.atom | ||
and node.type == syms.for_stmt | ||
and isinstance(child.prev_sibling, Leaf) | ||
and child.prev_sibling.type == token.NAME | ||
and child.prev_sibling.value == "for" | ||
): | ||
if maybe_make_parens_invisible_in_atom( | ||
child, | ||
parent=node, | ||
remove_brackets_around_comma=True, | ||
): | ||
wrap_in_parentheses(node, child, visible=False) | ||
elif preview and isinstance(child, Node) and node.type == syms.with_stmt: | ||
remove_with_parens(child, node) | ||
elif child.type == syms.atom: | ||
if maybe_make_parens_invisible_in_atom( | ||
child, | ||
parent=node, | ||
preview=preview, | ||
): | ||
wrap_in_parentheses(node, child, visible=False) | ||
elif is_one_tuple(child): | ||
|
@@ -866,38 +884,62 @@ def normalize_invisible_parens( | |
elif not (isinstance(child, Leaf) and is_multiline_string(child)): | ||
wrap_in_parentheses(node, child, visible=False) | ||
|
||
check_lpar = isinstance(child, Leaf) and child.value in parens_after | ||
comma_check = child.type == token.COMMA if preview else False | ||
|
||
check_lpar = isinstance(child, Leaf) and ( | ||
child.value in parens_after or comma_check | ||
) | ||
|
||
|
||
def remove_with_parens(node: Node, parent: Node) -> None: | ||
"""Recursively hide optional parens in `with` statements.""" | ||
if node.type == syms.atom: | ||
if maybe_make_parens_invisible_in_atom( | ||
node, | ||
parent=parent, | ||
remove_brackets_around_comma=True, | ||
): | ||
wrap_in_parentheses(parent, node, visible=False) | ||
if isinstance(node.children[1], Node): | ||
remove_with_parens(node.children[1], node) | ||
elif node.type == syms.testlist_gexp: | ||
for child in node.children: | ||
if isinstance(child, Node): | ||
remove_with_parens(child, node) | ||
elif node.type == syms.asexpr_test and not any( | ||
leaf.type == token.COLONEQUAL for leaf in node.leaves() | ||
): | ||
if maybe_make_parens_invisible_in_atom( | ||
node.children[0], | ||
parent=parent, | ||
remove_brackets_around_comma=True, | ||
): | ||
wrap_in_parentheses(parent, node.children[0], visible=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Basically the different variations of bracketed with statements give pretty different parse trees so we need to recursively solve this.
|
||
|
||
|
||
def maybe_make_parens_invisible_in_atom( | ||
node: LN, | ||
parent: LN, | ||
preview: bool = False, | ||
remove_brackets_around_comma: bool = False, | ||
) -> bool: | ||
"""If it's safe, make the parens in the atom `node` invisible, recursively. | ||
Additionally, remove repeated, adjacent invisible parens from the atom `node` | ||
as they are redundant. | ||
|
||
Returns whether the node should itself be wrapped in invisible parentheses. | ||
|
||
""" | ||
if ( | ||
preview | ||
and parent.type == syms.for_stmt | ||
and isinstance(node.prev_sibling, Leaf) | ||
and node.prev_sibling.type == token.NAME | ||
and node.prev_sibling.value == "for" | ||
): | ||
for_stmt_check = False | ||
else: | ||
for_stmt_check = True | ||
|
||
if ( | ||
node.type != syms.atom | ||
or is_empty_tuple(node) | ||
or is_one_tuple(node) | ||
or (is_yield(node) and parent.type != syms.expr_stmt) | ||
or (max_delimiter_priority_in_atom(node) >= COMMA_PRIORITY and for_stmt_check) | ||
or ( | ||
# This condition tries to prevent removing non-optional brackets | ||
# around a tuple, however, can be a bit overzealous so we provide | ||
# and option to skip this check for `for` and `with` statements. | ||
not remove_brackets_around_comma | ||
and max_delimiter_priority_in_atom(node) >= COMMA_PRIORITY | ||
) | ||
Comment on lines
+955
to
+961
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the reason I had the The issue was that nested brackets are nested atoms so when we recursed into the second level of this function the parent would no longer be |
||
): | ||
return False | ||
|
||
|
@@ -920,7 +962,11 @@ def maybe_make_parens_invisible_in_atom( | |
# make parentheses invisible | ||
first.value = "" | ||
last.value = "" | ||
maybe_make_parens_invisible_in_atom(middle, parent=parent, preview=preview) | ||
maybe_make_parens_invisible_in_atom( | ||
middle, | ||
parent=parent, | ||
remove_brackets_around_comma=remove_brackets_around_comma, | ||
) | ||
|
||
if is_atom_with_invisible_parens(middle): | ||
# Strip the invisible parens from `middle` by replacing | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
with (open("bla.txt")): | ||
JelleZijlstra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pass | ||
|
||
with (open("bla.txt")), (open("bla.txt")): | ||
pass | ||
|
||
with (open("bla.txt") as f): | ||
pass | ||
|
||
# Remove brackets within alias expression | ||
with (open("bla.txt")) as f: | ||
pass | ||
|
||
# Remove brackets around one-line context managers | ||
with (open("bla.txt") as f, (open("x"))): | ||
pass | ||
|
||
with ((open("bla.txt")) as f, open("x")): | ||
pass | ||
|
||
with (CtxManager1() as example1, CtxManager2() as example2): | ||
... | ||
|
||
# Brackets remain when using magic comma | ||
with (CtxManager1() as example1, CtxManager2() as example2,): | ||
... | ||
|
||
# Brackets remain for multi-line context managers | ||
with (CtxManager1() as example1, CtxManager2() as example2, CtxManager2() as example2, CtxManager2() as example2, CtxManager2() as example2): | ||
... | ||
|
||
# Don't touch assignment expressions | ||
with (y := open("./test.py")) as f: | ||
pass | ||
|
||
# output | ||
with open("bla.txt"): | ||
pass | ||
|
||
with open("bla.txt"), open("bla.txt"): | ||
pass | ||
|
||
with open("bla.txt") as f: | ||
pass | ||
|
||
# Remove brackets within alias expression | ||
with open("bla.txt") as f: | ||
pass | ||
|
||
# Remove brackets around one-line context managers | ||
with open("bla.txt") as f, open("x"): | ||
pass | ||
|
||
with open("bla.txt") as f, open("x"): | ||
pass | ||
|
||
with CtxManager1() as example1, CtxManager2() as example2: | ||
... | ||
|
||
# Brackets remain when using magic comma | ||
with ( | ||
CtxManager1() as example1, | ||
CtxManager2() as example2, | ||
): | ||
... | ||
|
||
# Brackets remain for multi-line context managers | ||
with ( | ||
CtxManager1() as example1, | ||
CtxManager2() as example2, | ||
CtxManager2() as example2, | ||
CtxManager2() as example2, | ||
CtxManager2() as example2, | ||
): | ||
... | ||
|
||
# Don't touch assignment expressions | ||
with (y := open("./test.py")) as f: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for
fix is now very simple. Detect the parent is afor
loop and then remove the brackets using the option to remove around commas as well.