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.

Sign up for GitHub

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

Jump to bottom

bpo-47062: Implement asyncio.Runner context manager #31799

Merged
merged 32 commits into from
Mar 24, 2022
Merged

bpo-47062: Implement asyncio.Runner context manager #31799

merged 32 commits into from
Mar 24, 2022

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Mar 10, 2022 •
edited by bedevere-bot
Loading

@asvetlov asvetlov changed the title asyncio.Runner context manager bpo-47062: asyncio.Runner context manager Mar 18, 2022
@asvetlov asvetlov changed the title bpo-47062: asyncio.Runner context manager bpo-47062: Implement asyncio.Runner context manager Mar 18, 2022
@asvetlov asvetlov marked this pull request as ready for review March 18, 2022 20:46
@asvetlov asvetlov requested a review from 1st1 as a code owner March 18, 2022 20:46
zware
zware reviewed Mar 18, 2022
View reviewed changes
Copy link
Member

@zware zware left a comment

Choose a reason for hiding this comment

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

I have a few mostly grammatical suggestions, but otherwise LGTM.

There may be a few comments that no longer apply; I started reviewing before you finished pushing changes and now I can't find them to remove them :)

Doc/library/asyncio-runner.rst Outdated Show resolved Hide resolved
Lib/asyncio/runners.py Outdated Show resolved Hide resolved
Doc/library/asyncio-runner.rst Outdated Show resolved Hide resolved
Lib/asyncio/runners.py Outdated Show resolved Hide resolved
Lib/test/test_asyncio/test_runners.py Show resolved Hide resolved
asvetlov and others added 5 commits March 19, 2022 00:11
@asvetlov @zware
Update Lib/asyncio/runners.py
546440b 
Co-authored-by: Zachary Ware <zach@python.org>
@asvetlov @zware
Update Doc/library/asyncio-runner.rst
6935f7d 
Co-authored-by: Zachary Ware <zach@python.org>
@asvetlov @zware
Update Lib/asyncio/runners.py
9b9a004 
Co-authored-by: Zachary Ware <zach@python.org>
@asvetlov
Improve wording
b0c5b8c
@asvetlov
Add a test for double 'with' usage
599c9db
@asvetlov
Copy link
Contributor Author

Thanks, @zware
Notes are fixed

1st1
1st1 reviewed Mar 21, 2022
View reviewed changes
Lib/asyncio/runners.py Outdated Show resolved Hide resolved
1st1
1st1 reviewed Mar 21, 2022
View reviewed changes
Lib/asyncio/runners.py Outdated Show resolved Hide resolved
@asvetlov
Copy link
Contributor Author

I've modified the Runner class to use lazy initialization: __enter__ or the first run() creates embedded loop and contextvars.Context.

@1st1 does it look better for you?

@asvetlov
Copy link
Contributor Author

Explicit runner.get_context() method is dropped.
An embedded context is still created at the time of embedded loop initialization.

@seifertm seifertm mentioned this pull request Mar 24, 2022
Closed
@asvetlov asvetlov merged commit 4119d2d into main Mar 24, 2022
@asvetlov asvetlov deleted the asyncio-runner branch March 24, 2022 19:51
@1st1
Copy link
Member

1st1 commented Mar 24, 2022

Post merge: can you rename "factory" to "loop_factory"?

@asvetlov asvetlov mentioned this pull request Mar 25, 2022
Merged
@seifertm seifertm mentioned this pull request Apr 5, 2022
Closed
@asvetlov asvetlov mentioned this pull request Jun 2, 2022
Closed
@Bluenix2 Bluenix2 mentioned this pull request Jul 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1st1 1st1 1st1 left review comments

@zware zware zware approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@asvetlov @1st1 @zware @the-knights-who-say-ni @bedevere-bot