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

Omit asyncio.timeout from SIM117 #8606

Closed
maltevesper opened this issue Nov 10, 2023 · 5 comments · Fixed by #8801
Closed

Omit asyncio.timeout from SIM117 #8606

maltevesper opened this issue Nov 10, 2023 · 5 comments · Fixed by #8801
Labels
bug Something isn't working

Comments

@maltevesper
Copy link
Contributor

While I generally agree with SIM117 (combine with statements where possible), I am wondering if there are cases where there should be exceptions to the rule, in particular asyncio.timeout comes to mind.

I.e. I find the following clearer

async with asyncio.timeout(0.2):
    async with line_processor:
        await line_processor.stop(0)

Less clear in my opinion:

async with asyncio.timeout(0.2), line_processor:
    await line_processor.stop(0)

While this is a minor cosmetic issue, I am wondering if this warrants a configuration option (a list of contextmanagers for which to ignore SIM117), or is to much of an edge case to consider complicating the configuration.

For now I ignore SIM117 for tests, which is the main case in which I trigger these.

@charliermarsh charliermarsh added the configuration Related to settings and configuration label Nov 11, 2023
@charliermarsh
Copy link
Member

This seems like a reasonable suggestion -- I agree that collapsing the timeout here is awkward, since semantically it's meant to be wrapping the inner async with rather than peer to it. We could start just by omitting asyncio.timeout, and parameterize it later on if there's more need.

@charliermarsh charliermarsh changed the title SIM117 ignore for asyncio.timeout? Omit asyncio.timeout from SIM117 Nov 11, 2023
@charliermarsh charliermarsh added the bug Something isn't working label Nov 11, 2023
@charliermarsh
Copy link
Member

PR welcome if you're interested :) The changes would be in multiple_with_statements, we'd want to check if the with_parent has a single item that matches asyncio.timeout.

@charliermarsh charliermarsh removed the configuration Related to settings and configuration label Nov 11, 2023
@zanieb
Copy link
Member

zanieb commented Nov 11, 2023

If we're going with the spirit of this change, we should probably also include AnyIO/Trio's move_on_after, fail_after, move_on_at, fail_at, and maybeCancelScope.

@maltevesper
Copy link
Contributor Author

I will happily have a go at a PR this week (am a little busy)

Locks crossed my mind as a possible further candidate for "more explicit scoping", but I will stick with timing for starters.

@charliermarsh
Copy link
Member

Awesome, just ask here or in Discord if you have any questions.

maltevesper added a commit to maltevesper/ruff that referenced this issue Nov 21, 2023
Semantically it makes sense to put certain contextmanagers
into separate with statements. Currently asyncio.timeout
and its relatives in anyio and trio are exempt from SIM117.

Closes astral-sh#8606
maltevesper added a commit to maltevesper/ruff that referenced this issue Nov 21, 2023
Semantically it makes sense to put certain contextmanagers
into separate with statements. Currently asyncio.timeout
and its relatives in anyio and trio are exempt from SIM117.

Closes astral-sh#8606
charliermarsh pushed a commit that referenced this issue Nov 21, 2023
Semantically it makes sense to put certain contextmanagers into separate
with statements. Currently asyncio.timeout and its relatives in anyio
and trio are exempt from SIM117.

Closes #8606

## Summary

Exempt asyncio.timeout and related functions from SIM117 (Collapse with
statements where possible).
See #8606 for more.

## Test Plan

Extended the insta tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants