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

refactor: Create AsyncExitStack using attrs factory #907

Closed
wants to merge 2 commits into from

Conversation

injust
Copy link

@injust injust commented May 12, 2024

A bit nicer to have attrs create the AsyncExitStack, instead of assigning self._exit_stack = exit_stack.pop_all() from within the with block.

@injust
Copy link
Author

injust commented May 12, 2024

Seems like the Windows tests are broken. Failures aren't relevant to my change and also happen in other PRs.

@agronholm
Copy link
Owner

You're changing the semantics for the worse. The reason I'm doing async with AsyncExitStack(): in __aenter__() is to ensure that the stack gets rewound if there's an unhandled exception during __aenter__(). This would no longer happen if I were to merge this PR.

@agronholm agronholm closed this May 12, 2024
@injust injust deleted the patch-1 branch May 12, 2024 10:15
@injust
Copy link
Author

injust commented May 12, 2024

You're changing the semantics for the worse. The reason I'm doing async with AsyncExitStack(): in __aenter__() is to ensure that the stack gets rewound if there's an unhandled exception during __aenter__(). This would no longer happen if I were to merge this PR.

Thanks for the explanation; I learned a thing today. Didn't think hard enough about exception handling within __aenter__().

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

Successfully merging this pull request may close these issues.

None yet

2 participants