bpo-34728: Remove deprecate *loop* argument in asyncio.sleep by joaojunior · Pull Request #9415 · python/cpython

joaojunior · 2018-09-19T10:29:14Z

Warn deprecate argument loop in asyncio.sleep, asyncio.wait and asyncio.wait_for. Also change calls for get_event_loop() to calls for get_running_loop() in these methods.

https://bugs.python.org/issue34728

the-knights-who-say-ni · 2018-09-19T10:29:17Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

fbidu · 2018-09-19T12:35:07Z

I think that just removing the loop argument may be bad for compatibility reasons. Perhaps it is better to catch the argument and raise an DeprecationWarning, as proposed in the issue

@fbidu very good. I will change for this and send the PR again. Thank you for your review.

Yes, @fbidu is right.

…p argument is used

joaojunior · 2018-09-20T18:44:20Z

Hi @fbidu and @1st1 I fix here. Also, I add test for this.
When I run all tests in my machine, it works. When run in the CI, doesn't work. Could you help me?

1st1 · 2018-09-20T18:47:09Z

        self.assertEqual(result, 11)

+    def test_loop_argument_is_deprecated(self):
+        """


Use a regular comment for this, not a docstring.

1st1 · 2018-09-20T18:48:15Z

+        will be removed
+        """
+        with self.assertWarns(DeprecationWarning):
+            self.loop.run_until_complete(asyncio.wait_for(coroutine_function(),


Weird formatting, I'd do this:

self.loop.run_until_complete( asyncio.wait_for(coroutine_function(), 0.01, loop=self.loop))

1st1 · 2018-09-20T18:50:09Z

When I run all tests in my machine, it works. When run in the CI, doesn't work. Could you help me?

Looks like CI failures are unrelated to this PR.

joaojunior · 2018-09-20T19:27:05Z

@1st1 Could you check again?

1st1 · 2018-09-20T19:40:14Z

Please add a news entry using blurb

joaojunior · 2018-09-20T20:27:09Z

@1st1 It is my first contribution in Python. :-)
Thank you for your help and comments.

1st1 · 2018-09-20T20:37:14Z

@asvetlov please take a look too.

@joaojunior BTW, I think we can also replace get_event_loop() calls with get_running_loop().

joaojunior · 2018-09-20T20:39:43Z

@1st1 Very good. I will change and run the tests to verify.

willingc

Thanks @joaojunior for this PR. I've made some small style suggestions should you wish to add them to the PR.

willingc · 2018-09-21T05:54:10Z

@@ -0,0 +1,2 @@
+Add warn when deprecate argument `loop` is used in methods: `asyncio.sleep`,


Perhaps a bit clearer: Add deprecation warning when loop is used ...

willingc · 2018-09-21T06:07:30Z

        self.assertEqual(result, 11)

+    def test_loop_argument_is_deprecated(self):
+        # this test should be removed in Python 4.0 when the loop argument


If possible, would you like to shorten the 2 line comment to 1 line in these tests?

Suggestion: Remove test when loop argument is removed in Python 4.0

Thanks for writing tests 😄

Hi @willingc Thanks for your comments. I will change now.

joaojunior · 2018-09-24T09:40:23Z

@willingc Thank you for your suggestions and help. I already change.

willingc

Looks good. Thanks for making the updates @joaojunior

ZackerySpytz · 2018-09-24T12:57:49Z

-        loop = events.get_event_loop()
+        loop = events.get_running_loop()
+    else:
+        warnings.warn("The loop argument is deprecated and scheduled for"


All of the warning messages are missing spaces (between "for" and "removal").

You're right. @joaojunior please submit another PR to fix this.

@willingc I don't think @asvetlov had a chance to review this PR :(

@1st1 Ok, I will send.

Oops. Sorry @1st and @asvetlov. I messed up on that. 😞

@joaojunior never mind, it's handled in #9579

asvetlov · 2018-09-25T21:33:02Z

Sorry, I have a pretty busy week.
Question: if we want to remove the parameter in 4.0 should we add PendingDeprecationWarning in 3.8 and DeprecationWarning in 3.9?

1st1 · 2018-09-25T21:36:37Z

I think it's OK to leave DeprecationWarning as it's just completely non-sensical to pass the loop argument. But if you feel strongly about this (I don't) please make a PR.

In this PR: python/cpython#9415 asyncio.sleep switched to requiring a running asyncio event loop. This is all very sensible, but broke our test that checks what happens when some accidentally yields an asyncio.Future to the Trio run loop. Switch the test to waiting on a Future directly.

asvetlov · 2018-09-26T05:28:43Z

I know many (old but still executed) codes that pass loop to sleep() and other functions explicitly.
I used to write it myself two or three years ago for example.
From my perspective, it is as non-sensical as passing bare coroutines into wait() / gather().

I don't have a strong preference but have a feeling that we need to be consistent in API deprecation.
A subjective criterion like "how many codes do we expect to break" (statistic doesn't dot exists because the codes are mostly not public) is very unobvious and not reliable.

joaojunior requested review from 1st1, asvetlov, gpshead and rhettinger as code owners September 19, 2018 10:29

the-knights-who-say-ni added the CLA not signed label Sep 19, 2018

bedevere-bot added the awaiting review label Sep 19, 2018

joaojunior changed the title ~~bpo34728: Remove deprecate *loop* argument in asyncio.sleep~~ bpo-34728: Remove deprecate *loop* argument in asyncio.sleep Sep 19, 2018

fbidu reviewed Sep 19, 2018

View reviewed changes

Joao Junior added 2 commits September 20, 2018 10:58

Insert the warn in the asyncio.sleep when the loop argument is used

ef8c264

Insert the warn in the asyncio.wait and asyncio.wait_for when the loo…

db2c478

…p argument is used

joaojunior force-pushed the remove-loop-argument-in-sleep branch from a015623 to db2c478 Compare September 20, 2018 09:39

1st1 reviewed Sep 20, 2018

View reviewed changes

1st1 removed request for gpshead and rhettinger September 20, 2018 18:48

Better format of the code

205fb6f

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 20, 2018

Add news file

0710bba

change calls for get_event_loop() to calls for get_running_loop()

3c1f283

1st1 approved these changes Sep 20, 2018

View reviewed changes

bedevere-bot removed the awaiting review label Sep 20, 2018

bedevere-bot added the awaiting merge label Sep 20, 2018

willingc approved these changes Sep 21, 2018

View reviewed changes

Joao Junior added 2 commits September 24, 2018 08:44

Change message to be more clear in News

9508e4f

Improve the comments in test_tasks

ad0cf25

willingc approved these changes Sep 24, 2018

View reviewed changes

willingc merged commit 558c49b into python:master Sep 24, 2018

bedevere-bot removed the awaiting merge label Sep 24, 2018

joaojunior deleted the remove-loop-argument-in-sleep branch September 24, 2018 10:03

ZackerySpytz reviewed Sep 24, 2018

View reviewed changes

njsmith mentioned this pull request Sep 26, 2018

Don't use asyncio.sleep as a function that waits on an asyncio.Future python-trio/trio#683

Merged

		@@ -0,0 +1,2 @@
		Add warn when deprecate argument `loop` is used in methods: `asyncio.sleep`,

Uh oh!

Conversation

joaojunior commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 19, 2018

Uh oh!

fbidu Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaojunior commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Sep 20, 2018

Uh oh!

joaojunior commented Sep 20, 2018

Uh oh!

1st1 commented Sep 20, 2018

Uh oh!

joaojunior commented Sep 20, 2018

Uh oh!

1st1 commented Sep 20, 2018

Uh oh!

joaojunior commented Sep 20, 2018

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaojunior commented Sep 24, 2018

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Sep 25, 2018

Uh oh!

1st1 commented Sep 25, 2018

Uh oh!

asvetlov commented Sep 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

joaojunior commented Sep 19, 2018 •

edited

Loading

fbidu Sep 19, 2018 •

edited

Loading

joaojunior commented Sep 20, 2018 •

edited

Loading