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

bpo-42392: Mention loop removal in whatsnew for 3.10 #24256

Merged
merged 10 commits into from Jan 21, 2021

Conversation

@Fidget-Spinner
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner commented Jan 19, 2021

@vstinner noticed on python-dev that there is no what's new or porting entry for removal of asyncio loop parameter.

This patch adds a basic guide.

Co-Authored-By: Kyle Stanley aeros167@gmail.com

https://bugs.python.org/issue42392

Automerge-Triggered-By: GH:aeros

@Fidget-Spinner
Copy link
Contributor Author

@Fidget-Spinner Fidget-Spinner commented Jan 19, 2021

Personally, I feel that we should mention that dropping support for 3.6 altogether in user code is always an option - 3.6's expected last security release is in 2021-12, and 3.10's release date is 2021-10-04. So when 3.10's out, only 2 months are left until 3.6's expected EOL arrives anyways.

Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
@1st1
1st1 approved these changes Jan 19, 2021
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Copy link
Member

@aeros aeros left a comment

Thanks for the PR @Fidget-Spinner! This definitely is worth including to provide users with more information about the removal of the loop parameter throughout many asyncio functions. However, I'd like to suggest a few changes to make it more specific and give users some context about why it was removed.

Also, sorry in advance if I'm slow to respond, as I'm presently still on an open source break. I just considered this issue important enough to provide some feedback given the widespread impact on asyncio users.

Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jan 19, 2021

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

async def foo(loop):
await asyncio.sleep(loop=loop)

Can *usually* be replaced with this::

This comment has been minimized.

@briancurtin

briancurtin Jan 19, 2021
Member

Are there actually cases where this example is not the solution and another example should be provided? The APIs that I'm familiar with that got changed would be solved by this in every case, but maybe there are some that require a different fix?

If not, I'd remove the *usually* so there's no confusion about what else might need to be done.

This comment has been minimized.

@aeros

aeros Jan 20, 2021
Member

It specifically doesn't apply if the user wants to utilize an event loop other than the currently running one. But, for 90% of use cases, the user will want to use the running event loop anyways (that's largely why the loop arg was removed in the first place). I expanded upon this in another comment.

Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
@Fidget-Spinner
Copy link
Contributor Author

@Fidget-Spinner Fidget-Spinner commented Jan 20, 2021

Thanks for the PR @Fidget-Spinner! This definitely is worth including to provide users with more information about the removal of the loop parameter throughout many asyncio functions. However, I'd like to suggest a few changes to make it more specific and give users some context about why it was removed.

@aeros Thank you so much for the reviews! They were extremely illuminating and constructive :).

Also, sorry in advance if I'm slow to respond, as I'm presently still on an open source break.

No worries, please enjoy your break!

@Fidget-Spinner
Copy link
Contributor Author

@Fidget-Spinner Fidget-Spinner commented Jan 20, 2021

For bedevere-bot: I have made the requested changes; please review again. Thank you!

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jan 20, 2021

Thanks for making the requested changes!

@1st1, @aeros: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from 1st1 Jan 20, 2021
@bedevere-bot bedevere-bot requested a review from aeros Jan 20, 2021
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

LGTM. @aeros: do you want to have a second look or merge the PR?

@aeros
aeros approved these changes Jan 20, 2021
Copy link
Member

@aeros aeros left a comment

Thanks for making the suggested changes @Fidget-Spinner! I very much like the way the motivation section succinctly reads and think readers will benefit greatly from the additional information. :-)

I just have a couple very minor nits and then I can proceed with merging.

Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
@miss-islington miss-islington merged commit dcea78f into python:master Jan 21, 2021
11 checks passed
11 checks passed
Docs
Details
Check for source changes
Details
Check if generated files are up to date
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20210120.70 succeeded
Details
Travis CI - Pull Request Build Passed
Details
bedevere/issue-number Issue number 42392 found
Details
bedevere/news "skip news" label found
@Fidget-Spinner Fidget-Spinner deleted the Fidget-Spinner:doc-loop branch Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants