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-34790: Implement deprecation of passing coroutines to asyncio.wait() #16977

Merged

Conversation

@aeros
Copy link
Member

aeros commented Oct 29, 2019

@aeros

This comment has been minimized.

Copy link
Member Author

aeros commented Oct 29, 2019

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 aeros added the DO-NOT-MERGE label Oct 29, 2019
@aeros

This comment has been minimized.

Copy link
Member Author

aeros commented Oct 29, 2019

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

@aeros aeros removed the DO-NOT-MERGE label Oct 29, 2019
Copy link
Contributor

asvetlov left a comment

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

This comment has been minimized.

Copy link
Member Author

aeros commented Oct 29, 2019

@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

This comment has been minimized.

Copy link
Member Author

aeros commented Oct 29, 2019

@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

This comment has been minimized.

Copy link
Member Author

aeros commented Dec 30, 2019

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

@asvetlov asvetlov merged commit 89aa7f0 into python:master Dec 30, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20191029.13 succeeded
Details
bedevere/issue-number Issue number 34790 found
Details
bedevere/news "skip news" label found
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asvetlov

This comment has been minimized.

Copy link
Contributor

asvetlov commented Dec 30, 2019

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

This comment has been minimized.

Copy link
Member Author

aeros commented Dec 30, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.