FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-34790: Implement deprecation of passing coroutines to asyncio.wait() by aeros · Pull Request #16977 · python/cpython · GitHub

Repository navigation

bpo-34790: Implement deprecation of passing coroutines to asyncio.wait() - #16977

Merged
asvetlov merged 5 commits into
python:masterfrom
aeros:bpo34790-implement-asyncio.wait-coro-deprecation
Dec 30, 2019
Merged

asvetlov merged 5 commits into
python:masterfrom
aeros:bpo34790-implement-asyncio.wait-coro-deprecation

Conversation

aeros commented Oct 29, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

aeros commented Oct 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Adding a DO-NOT-MERGE label for now. Although the deprecation seems to be properly implemented, I have to refactor the existing tests with self.assertWarns(DeprecationWarning) where coroutines are being passed to asyncio.wait().

aeros commented Oct 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I have to refactor the existing tests with self.assertWarns(DeprecationWarning) where coroutine objects are being passed to asyncio.wait()

Done, removing DO-NOT-MERGE label.

[aeros:~/repos/aeros-cpython]$ ./python -W error -m test test_asyncio
0:00:00 load avg: 0.33 Run tests sequentially
0:00:00 load avg: 0.33 [1/1] test_asyncio
test_asyncio passed in 43.2 sec

== Tests result: SUCCESS ==

1 test OK.

Total duration: 43.2 sec
Tests result: SUCCESS

asvetlov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.
The PR should not be backported to 3.8, right?
It raises new deprecations that were absent in 3.8.0; adding them in 3.8.1 is confusing.

aeros commented Oct 29, 2019

Copy link
Copy Markdown
Contributor Author

@asvetlov

The PR should not be backported to 3.8, right?

Yep, that's why I did this one separately from #16975, which only fixed the 3.8 whatsnew entry based on Yury's suggestion. Otherwise, the changes would've been combined into the same PR.

It raises new deprecations that were absent in 3.8.0; adding them in 3.8.1 is confusing.

Agreed, I'm starting to realize that we should try to avoid implementing significant deprecations partway through a major version (3.x) when possible. It would more likely comely across to users as a surprise and lead to some confusion rather than being overly helpful. For example, if users have already ran their tests with warnings enabled and resolved any deprecation warnings for 3.8.0, I don't think it would be a great experience for them to encounter a bunch of new ones in 3.8.1.

aeros commented Oct 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@1st1 @asvetlov What are your thoughts on updating the documentation for asyncio.as_completed() to mention that an iterator of completed coroutine objects is returned when coroutine objects are passed to fs? I don't think this behavior is very obvious.

As a result, I would like to structure it similarly to the documentation for asyncio.ensure_future(), where the behavior for Futures, Tasks, and coroutine objects are explicitly specified.

For more context, see #16977 (comment).

aeros commented Dec 30, 2019

Copy link
Copy Markdown
Contributor Author

@asvetlov @1st1 Is there anything else that needs to added to this PR or are we just waiting for a final review?

asvetlov merged commit 89aa7f0 into python:master Dec 30, 2019

Copy link
Copy Markdown
Contributor

Thanks.
Regarding mentioned docs for as_completed() and ensure_future() -- please let's discuss in separate issues.
I quite don't follow your proposals, sorry.

aeros commented Dec 30, 2019

Copy link
Copy Markdown
Contributor Author

Regarding mentioned docs for as_completed() and ensure_future() -- please let's discuss in separate issues.

Yeah, I'll have to spend more time fleshing those ideas out and consider if the proposals might be worthwhile.

Thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL