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
Contributor

@aeros aeros commented Oct 29, 2019

@aeros
Copy link
Contributor 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
Copy link
Contributor 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

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor 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
Copy link
Contributor 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
Copy link
Contributor 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
@asvetlov
Copy link
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
Copy link
Contributor 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
Development

Successfully merging this pull request may close these issues.

4 participants