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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SIM117] Merge with statements for context managers that have same scope #35

Closed
Skylion007 opened this issue Jan 30, 2021 · 5 comments · Fixed by #36
Closed

[SIM117] Merge with statements for context managers that have same scope #35

Skylion007 opened this issue Jan 30, 2021 · 5 comments · Fixed by #36
Assignees
Labels
enhancement New feature or request

Comments

@Skylion007
Copy link
Contributor

Skylion007 commented Jan 30, 2021

Explanation

A little known feature of Python is that a with statement can contain multiple context managers. This allows the code only one with statement (and therefore only 1 level of indentation). This rule has a similar rational to SIM102.

This rule should be applied if and only if:

  • context A only contains the code of Context B and no other code. That is all the code in the nested statement will run with context A and B.

Caveat when implementing: If the context names are really long, the with statement may be broken over a line break. A new feature in the Python 3.10 alpha will be to allow parentheses to be used to break the with statement over multiple lines.

Example

Consider the following context managers:

#bad
with A() as a:
    with B() as b:
        print('hello')

can be transformed into the following:

# Good
with A() as a, B() as b:
    print('hello')
@Skylion007 Skylion007 added the enhancement New feature or request label Jan 30, 2021
@MartinThoma
Copy link
Owner

I love it! Should be easy to implement. I hope to find the time for it tomorrow :-)

@MartinThoma
Copy link
Owner

$ astpretty --no-show-offsets /dev/stdin <<< `cat example.txt`
Module(
    body=[
        With(
            items=[
                withitem(
                    context_expr=Call(
                        func=Name(id='A', ctx=Load()),
                        args=[],
                        keywords=[],
                    ),
                    optional_vars=Name(id='a', ctx=Store()),
                ),
            ],
            body=[
                With(
                    items=[
                        withitem(
                            context_expr=Call(
                                func=Name(id='B', ctx=Load()),
                                args=[],
                                keywords=[],
                            ),
                            optional_vars=Name(id='b', ctx=Store()),
                        ),
                    ],
                    body=[
                        Expr(
                            value=Call(
                                func=Name(id='print', ctx=Load()),
                                args=[Constant(value='hello', kind=None)],
                                keywords=[],
                            ),
                        ),
                    ],
                    type_comment=None,
                ),
            ],
            type_comment=None,
        ),
    ],
    type_ignores=[],
)

@MartinThoma MartinThoma changed the title [New Rule] Merge with statements for context managers that have same scope [SIM117] Merge with statements for context managers that have same scope Jan 31, 2021
MartinThoma added a commit that referenced this issue Jan 31, 2021
MartinThoma added a commit that referenced this issue Jan 31, 2021
MartinThoma added a commit that referenced this issue Jan 31, 2021
MartinThoma added a commit that referenced this issue Jan 31, 2021
@MartinThoma
Copy link
Owner

@Skylion007 I've just added a PR which does the trick. I would also write you in the README for that rule. What do you think about that? I can also adjust it a little bit.

I want to make contributions like yours a bit more visible.

@Skylion007
Copy link
Contributor Author

Thanks for including me in the ReadMe. Left a minor nit on the PR, but otherwise looks good!

MartinThoma added a commit that referenced this issue Jan 31, 2021
MartinThoma added a commit that referenced this issue Jan 31, 2021
@MartinThoma
Copy link
Owner

Merged and released :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants