-
-
Notifications
You must be signed in to change notification settings - Fork 209
Fold nested context managers. #1012
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import ast | ||
| import functools | ||
| import itertools | ||
| from collections.abc import Iterable | ||
| from typing import Any | ||
|
|
||
| from tokenize_rt import Offset | ||
| from tokenize_rt import Token | ||
|
|
||
| from pyupgrade._ast_helpers import ast_to_offset | ||
| from pyupgrade._data import register | ||
| from pyupgrade._data import State | ||
| from pyupgrade._data import TokenFunc | ||
| from pyupgrade._token_helpers import Block | ||
|
|
||
|
|
||
| def _expand_item(indent: int, item: ast.AST) -> str: | ||
| return '{}{}'.format(' ' * indent, ast.unparse(item)) | ||
|
Owner
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. ast.unparse does not roundtrip so it is not usable or acceptable 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. https://github.com/Instagram/LibCST is finally being maintained again so I guess @shaleh could use this. Alternatively, and likely recommended by @asottile, use his https://github.com/asottile/tokenize-rt |
||
|
|
||
|
|
||
| def _replace_context_managers( | ||
| i: int, | ||
| tokens: list[Token], | ||
| *, | ||
| with_items: list[ast.withitem], | ||
| body: Iterable[ast.AST], | ||
| ) -> None: | ||
| block = Block.find(tokens, i, trim_end=True) | ||
| block_indent = block._minimum_indent(tokens) | ||
| replacement = '{}with ({}):\n{}\n'.format( | ||
| ' ' * block._initial_indent(tokens), | ||
| ', '.join(ast.unparse(item) for item in with_items), | ||
| '\n'.join(_expand_item(block_indent, item) for item in body), | ||
| ) | ||
| tokens[block.start:block.end] = [Token('CODE', replacement)] | ||
|
|
||
|
|
||
| def _drop_underscore_names(items: list[ast.withitem]) -> list[ast.withitem]: | ||
| """ | ||
| Remove unnecessary "_" names. | ||
|
|
||
| Returns an empty list if there are no names that need changing. | ||
| """ | ||
| transformed = [] | ||
| changed = False | ||
| for item in items: | ||
| if ( | ||
| isinstance(item.optional_vars, ast.Name) and | ||
| item.optional_vars.id == '_' | ||
| ): | ||
| item.optional_vars = None | ||
| changed = True | ||
| transformed.append(item) | ||
|
|
||
| if changed: | ||
| return transformed | ||
|
|
||
| return [] | ||
|
|
||
|
|
||
| def flatten(xs: Iterable[Any]) -> list[Any]: | ||
| return list(itertools.chain.from_iterable(xs)) | ||
|
Comment on lines
+40
to
+41
Owner
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. this is not at all acceptable or typesafe |
||
|
|
||
|
|
||
| @register(ast.With) | ||
| def visit_With_fold_nested( | ||
| state: State, | ||
| node: ast.With, | ||
| parent: ast.AST, | ||
| ) -> Iterable[tuple[Offset, TokenFunc]]: | ||
| """ | ||
| Fold nested with statements into one statement. | ||
|
|
||
| with foo: | ||
| with bar: | ||
| body | ||
|
|
||
| becomes | ||
|
|
||
| with (foo, bar): | ||
| body | ||
| """ | ||
| if state.settings.min_version < (3, 10): | ||
| return | ||
|
|
||
| with_stmts = [] | ||
| current: ast.AST = node | ||
| while True: | ||
| if isinstance(current, ast.With): | ||
| with_stmts.append(current) | ||
| if len(current.body) == 1: | ||
| current = current.body[0] | ||
| continue | ||
| break | ||
|
|
||
| if len(with_stmts) > 1: | ||
| with_items = flatten(n.items for n in with_stmts) | ||
| yield ast_to_offset(node), functools.partial( | ||
| _replace_context_managers, | ||
| body=with_stmts[-1].body, | ||
| with_items=with_items, | ||
| ) | ||
|
|
||
|
|
||
| @register(ast.With) | ||
| def visit_With_drop_unnecessary_underscore_names( | ||
| state: State, | ||
| node: ast.With, | ||
| parent: ast.AST, | ||
| ) -> Iterable[tuple[Offset, TokenFunc]]: | ||
| """ | ||
| Drop unnecessary _ names. | ||
|
|
||
| If this is a with statement with multiple items, remove any `as _`. | ||
| This was a work around before 3.10. | ||
|
|
||
| with (foo as _, bar as _): | ||
| body | ||
|
|
||
| becomes | ||
|
|
||
| with (foo, bar): | ||
| body | ||
| """ | ||
| if state.settings.min_version < (3, 10): | ||
| return | ||
|
|
||
| with_items = _drop_underscore_names(node.items) | ||
| if with_items: | ||
| yield ast_to_offset(node), functools.partial( | ||
| _replace_context_managers, | ||
| body=node.body, | ||
| with_items=with_items, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| from pyupgrade._data import Settings | ||
| from pyupgrade._main import _fix_plugins | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ('s', 'version'), | ||
| ( | ||
| pytest.param( | ||
| 'with foo:\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='simple with expression', | ||
| ), | ||
| pytest.param( | ||
| 'with foo as bar:\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='simple with expression and captured name', | ||
| ), | ||
| pytest.param( | ||
| 'with (foo as thing1, bar as thing2):\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='simple with expression and captured names', | ||
| ), | ||
| pytest.param( | ||
| 'with (foo as _, bar as _):\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| (3, 9), | ||
| id='nested with expression with empty name capture workaround', | ||
| ), | ||
| ), | ||
| ) | ||
| def test_fold_nested_context_managers_noop(s, version): | ||
| assert _fix_plugins(s, settings=Settings(min_version=version)) == s | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ('s', 'expected', 'version'), | ||
| ( | ||
| pytest.param( | ||
| 'with foo:\n' | ||
| ' with bar:\n' | ||
| " print('something')\n" | ||
| " print('another')\n" | ||
| '\n', | ||
| 'with (foo, bar):\n' | ||
| " print('something')\n" | ||
| " print('another')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='nested with expression', | ||
| ), | ||
| pytest.param( | ||
| 'if value:\n' | ||
| ' with foo:\n' | ||
| ' with bar:\n' | ||
| " print('something')\n" | ||
| " print('another')\n" | ||
| '\n', | ||
| 'if value:\n' | ||
| ' with (foo, bar):\n' | ||
| " print('something')\n" | ||
| " print('another')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='nested with expression inside of an if', | ||
| ), | ||
| pytest.param( | ||
| 'with foo as thing1:\n' | ||
| ' with bar as thing2:\n' | ||
| " print('something')\n" | ||
| " print('another')\n" | ||
| '\n', | ||
| 'with (foo as thing1, bar as thing2):\n' | ||
| " print('something')\n" | ||
| " print('another')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='nested with expression with named capture', | ||
| ), | ||
| pytest.param( | ||
| 'with (foo as _, bar as _):\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| 'with (foo, bar):\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='nested with expression with unnecessary empty name capture workaround', # noqa: E501 | ||
| ), | ||
| pytest.param( | ||
| 'with (foo as _, bar as thing2):\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| 'with (foo, bar as thing2):\n' | ||
| " print('something')\n" | ||
| '\n', | ||
| (3, 10), | ||
| id='nested with expression with one unnecessary empty name capture workaround', # noqa: E501 | ||
| ), | ||
| ), | ||
| ) | ||
| def test_fold_nested_context_managers(s, expected, version): | ||
| ret = _fix_plugins(s, settings=Settings(min_version=version)) | ||
| assert ret == expected |
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.
I don't think this is in scope. there's no reason to write the original code with nonsense as
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.
This was a real work around. I know, I maintain code with it. So I added code to remove it.
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.
It is also useless assigning to underscore so also fits in with things like replacing
set()with{}or similar which pyupgrade does.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.
no it does not. nobody would write code like that because it doesn't help
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.
https://docs.python.org/3.3/reference/compound_stmts.html#the-with-statement
You needed to use the
assyntax to stack them in onewith. But since you often did not need the name using_was a real thing.The options were:
or
As you correctly point out, the underscore names are not helpful and with 3.10 there is nothing forcing the programmer to use a name. So it is ok to remove them. Technically.... we could do analysis of the body and following code to see if any of the names are used but that gets ugly fast so I stuck with only removing the useless underscore.
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.
Disagree but understood. Will post a revised version shortly.
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.
In case we misunderstood each other.
3.3 to 3.9 did require the use of
asto nest context managers.with foo as f, bar as b:was the only way.3.10 remove the requirement for the
asnaming. Now you can dowith (foo, bar):and it follows the same pattern as import statements.My original PR removed the now unneeded syntax of the
aswhen migrating to 3.10 when the name was an underscore because that is was semantically safe. Any other name may have actually been used. I am moving this logic to another branch because I still want to upgrade code that I have to maintain elsewhere. I can post it as a separate PR for clarity. Or it can remain in a private repo. No worries.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.
no it did not. I just tried it and it works fine without as
the only thing 3.9 added was trailing commas
Uh oh!
There was an error while loading. Please reload this page.
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.
Hmm, our code was 3.7 at one point. I wonder if this was changed somewhere before 3.9. Because I know it was failing CI or we would not have added the ugly captures. Either way, it is out of the current PR. Folding the nesting is still valuable.
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.
Ok, I did some digging. Looks like that weird underscore form came from a StackOverflow discovery back in like 3.3 or 3.5 days.... definitely worthless now.
This code below passes 3.9 but the parenthesis form at the end does not parse in 3.8 or older. The
assyntax does not help.Thanks for your patience. I have removed one more mislearned thing from my list.