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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary parentheses from with statements #2926

Merged
merged 24 commits into from Apr 3, 2022

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Mar 15, 2022

Description

Closes #2921 and closes #2952. Aims to remove redundant parentheses from with statements:
Screen-Recording-2022-03-15-at-17 20 39

I currently have one failing test tests/data/parenthesized_context_managers.py since this code now removes unnecessary parentheses around one-line with statements, so think I'll just need to update the test to reflect this.
I tried looking in the contributing docs but couldn't see any info on how the tests work so please could you advise on where I should add this?

This is my first time contributing to black so let me know if there's anything I've missed 馃槃

Thanks!

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Mar 15, 2022

Thanks for your contribution! A few quick things:

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 15, 2022

thanks, I've got the tests working now and will refactor slightly to account for the preview mode

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 15, 2022

@JelleZijlstra I've updated the code to only remove the brackets in preview mode now 馃槃

@github-actions
Copy link

github-actions bot commented Mar 17, 2022

diff-shades reports zero changes comparing this PR (99167c2) to main (1af29fb).


What is this? | Workflow run | diff-shades documentation

src/black/linegen.py Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra self-assigned this Mar 24, 2022
Copy link
Collaborator

@ichard26 ichard26 left a comment

This case is unstable unfortunately:

with (((open("test"))) as a):
    pass
Mode(target_versions={<TargetVersion.PY37: 7>, <TargetVersion.PY38: 8>, <TargetVersion.PY36: 6>}, line_length=88, string_normalization=True, is_pyi=False, is_ipynb=False, magic_trailing_comma=True, experimental_string_processing=False, python_cell_magics=set(), preview=True)
--- source
+++ first pass
@@ -1,2 +1,2 @@
-with (((open("test"))) as a):
+with ((open("test"))) as a:
     pass
--- first pass
+++ second pass
@@ -1,2 +1,2 @@
-with ((open("test"))) as a:
+with open("test") as a:
     pass

And yes, we probably have to fix cases like these even if they're a bit out there :)

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 24, 2022

Classic, people spend ages requesting brackets around context managers and then they go and add the ability to add as many brackets as you want! 馃構

Will take a look tomorrow/this weekend 馃憤

Copy link
Contributor Author

@jpy-git jpy-git left a comment

@JelleZijlstra @ichard26 I've fixed both for and with bracket removals to work in one pass now 馃槃

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
)
Copy link
Contributor Author

@jpy-git jpy-git Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the reason I had the for_stmt_check/with_stmt_check initially was to disable this check for the reason in the above comment (basically it's hard to cleanly detect a tuple).

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 for_stmt or with_stmt so my check would fail and require a second run. I've added an argument to disable this check and then we can just use this when we're removing brackets from for/with in the main loop.

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)
Copy link
Contributor Author

@jpy-git jpy-git Mar 25, 2022

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 a for loop and then remove the brackets using the option to remove around commas as well.

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)
Copy link
Contributor Author

@jpy-git jpy-git Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with is slightly more complicated to achieve in one pass so I've split this into it's own function.

Basically the different variations of bracketed with statements give pretty different parse trees so we need to recursively solve this.

with (open("test.txt")) as f: ...: this is an asexpr_test
with (open("test.txt") as f): ...: this is an atom containing an asexpr_test
with (open("test.txt")) as f, (open("test.txt")) as f: ...: this is asexpr_test, COMMA, asexpr_test
with (open("test.txt") as f, open("test.txt")) as f: ...: this is an atom containing a testlist_gexp which then contains multiple asexpr_tests

@ichard26 ichard26 self-requested a review Mar 26, 2022
@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 26, 2022

One thing that I've noticed this PR doesn't currently do is add optional brackets if there aren't already some there.

So we don't correct

with ksjdhfdskfhdskfhdskjhfdsakjhfdskjhfdskjhfdskhfdskjhfdkjshfsdakjhf, ksjdhfdskfhdskfhdskjhfdsakjhfdskjhfdskjhfdskhfdskjhfdkjshfsdakjhf:
    pass

to

with (
    ksjdhfdskfhdskfhdskjhfdsakjhfdskjhfdskjhfdskhfdskjhfdkjshfsdakjhf,
    ksjdhfdskfhdskfhdskjhfdsakjhfdskjhfdskjhfdskhfdskjhfdkjshfsdakjhf,
):
    pass

The reason is due to the slightly odd way that with is parsed when there are no brackets vs other similar cases

with foo, bar:
    pass

[Leaf(NAME, 'with'), Leaf(NAME, 'foo'), Leaf(COMMA, ','), Leaf(NAME, 'bar'), Leaf(COLON, ':'), Node(suite, [Leaf(NE...ENT, '')])]
whereas in similar cases (e.g. for foo, bar in d.items(): ...) we would expect the comma separated list to be grouped as children of some node (e.g. exprlist). Therefore we can't add these brackets within this loop and have to put invisible parens around multiple children.

It's not too bad to add these brackets, I've got some code locally thats working, but this does lead me to ask:

  • If I did add the ability to add these brackets in and therefore fix long with lines, this would break python<3.9 code since these brackets weren't introduced until 3.9. Is there a way to have some logic occur only if we know the code being fixed is >=3.9?

The current behaviour without this additional change is basically "only fix brackets if they are already there since we know it must be >=3.9" so there's no risk of breaking code atm, but wanted to get your opinion on whether we could/should handle the no bracket case as well? 馃槃

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Mar 26, 2022

We should add these parentheses, but probably in a separate PR. There is already a framework for doing this based on versions; that's why we have the --target-version config option.

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 26, 2022

Fab thanks, just wanted to check/make you aware whilst reviewing this PR. Will follow up with a separate PR to add those in once we've finished this one 馃槃

@ichard26
Copy link
Collaborator

ichard26 commented Mar 26, 2022

This has been discussed in #664 and #1948 FYI.

@jpy-git jpy-git requested a review from JelleZijlstra Apr 2, 2022
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Thanks! @ichard26 did you want to do another review?

Copy link
Collaborator

@ichard26 ichard26 left a comment

Great clean work once again, thank you so much! #NewBlack

@JelleZijlstra JelleZijlstra merged commit 24c708e into psf:main Apr 3, 2022
40 checks passed
@jpy-git jpy-git deleted the with_parens branch Apr 3, 2022
felix-hilden added a commit to felix-hilden/black that referenced this pull request Jun 24, 2022
ichard26 added a commit that referenced this pull request Jun 27, 2022
Covers GH-2926, GH-2990, GH-2991, and GH-3035.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants