GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-34790: Implement deprecation of passing coroutines to asyncio.wait() by aeros · Pull Request #16977 · python/cpython

Conversation

@aeros

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

/cc @1st1 @asvetlov

https://bugs.python.org/issue34790

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
aeros added 3 commits October 28, 2019 20:58
@aeros
Implement deprecation of passing coros to asyncio.wait()
6e30ac4
@aeros
Add 3.9 whatsnew entry
b441b27
@aeros
Fix deprecation message
42caba2
@aeros
aeros requested review from 1st1 and asvetlov as code owners October 29, 2019 01:56
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 29, 2019
@bedevere-bot bedevere-bot added the awaiting review label Oct 29, 2019
@aeros aeros added skip news topic-asyncio labels Oct 29, 2019
@aeros

aeros commented Oct 29, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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().

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aeros aeros added the DO-NOT-MERGE label Oct 29, 2019
@aeros
Add self.assertWarns()
0e89978
@aeros

aeros commented Oct 29, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aeros aeros removed the DO-NOT-MERGE label Oct 29, 2019
aeros
aeros commented Oct 29, 2019
View reviewed changes
Comment thread Lib/test/test_asyncio/test_tasks.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@aeros
Trim comments
9a1d2bb
asvetlov
asvetlov approved these changes Oct 29, 2019
View reviewed changes

@asvetlov asvetlov left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aeros reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Oct 29, 2019
@aeros

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aeros

aeros commented Oct 29, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aeros

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asvetlov
asvetlov merged commit 89aa7f0 into python:master Dec 30, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 30, 2019
@asvetlov

asvetlov commented 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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sthagen sthagen mentioned this pull request Dec 30, 2019
bpo-34790: Implement deprecation of passing coroutines to asyncio.wai… sthagen/python-cpython#128
Merged
@aeros

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@aeros @shihai1991
bpo-34790: Implement deprecation of passing coroutines to asyncio.wai…
93b076d
…t() (pythonGH-16977)
@Jackenmen Jackenmen mentioned this pull request May 8, 2020
bpo-34790: add version of removal of explicit passing of coros to asyncio.wait's documentation #20008
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov asvetlov approved these changes
@1st1 1st1 Awaiting requested review from 1st1

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@aeros @asvetlov @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.