-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [streams] #13671
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
Conversation
This PR deprecate explicit loop parameters in all public asyncio APIs This issues is split to be easier to review. Second step: streams.py
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.
Please remove all deprecations in public API but add these warnings to open_connection()
, start_server()
, open_unix_connection()
, start_unix_server()
instead.
Lib/asyncio/streams.py
Outdated
@@ -753,6 +761,9 @@ def __init__(self, limit=_DEFAULT_LIMIT, loop=None): | |||
self._loop = events.get_event_loop() | |||
else: | |||
self._loop = loop | |||
warnings.warn("The loop argument is deprecated since Python 3.8, " |
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.
Private API, people should never create StreamReader class explicitly but use await open_connection()
and family.
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.
removed it
Lib/asyncio/streams.py
Outdated
@@ -1297,6 +1308,9 @@ def __init__(self, mode, *, | |||
self._loop = events.get_event_loop() | |||
else: | |||
self._loop = loop | |||
warnings.warn("The loop argument is deprecated since Python 3.8, " |
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.
Private API
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.
removed it
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again |
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
@asvetlov the News Could be skipped? |
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.
Please run your PR by ./python -Weeror -m test -v test_asyncio
.
I suspect that some tests will fail (but didn't check it myself).
These tests should be wrapped by with self.assertWarns(DeprecationWarning): ...
construction
Lib/asyncio/streams.py
Outdated
@@ -610,6 +626,9 @@ class StreamReaderProtocol(FlowControlMixin, protocols.Protocol): | |||
|
|||
def __init__(self, stream_reader, client_connected_cb=None, loop=None, | |||
*, _asyncio_internal=False): | |||
warnings.warn("The loop argument is deprecated since Python 3.8, " |
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.
This class is actually private API. Please remove the warning, I pretty sure that asyncio factories like open_connection
always send the loop
. Double deprecation is redundant.
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.
If so it should be documented as private, it does not appear to be : https://docs.python.org/3.6/library/asyncio-stream.html#streamreaderprotocol
And a quick search returns case of people passing the loop argument to it.
https://github.com/ssfdust/pyclewn/blob/555f2e5485f9938078adfe19388a0e1bb8af67f5/lib/clewn/tty.py#L119
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.
Agree, it should be documented properly.
I've added _asyncio_internal
flag.
Also, asincio.__init__
has __getattr__()
now to raise DeprecationWarning
for importing StreamReader
, StreamWriter
and StreamReaderProtocol
.
We have an agreement with @1st1 to land asyncio streams code changes first and document them later before Release Candidate stage.
I supposed to make a PR for documentation. If somebody would help me -- I very appreciate this.
Especially taking into account that my English is rough :(
Usually Yuri replaces almost every my sentence on review :)
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.
Hi @asvetlov I will help with the docs. :-)
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
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.
please drop the check here
I have made the requested changes; please review again |
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
@asvetlov I run the
|
Sorry, I can't merge this PR. Reason: |
Thanks @eamanu for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
GH-13833 is a backport of this pull request to the 3.8 branch. |
Thank you @eamanu The next big target is asyncio documentation cleanup (plus I need to land multithreaded support back again). |
@eamanu would you take a look on |
…Is [streams] (GH-13671) This PR deprecate explicit loop parameters in all public asyncio APIs This issues is split to be easier to review. Second step: streams.py https://bugs.python.org/issue36373 (cherry picked from commit 6d64a8f) Co-authored-by: Emmanuel Arias <emmanuelarias30@gmail.com>
Thank you. No rush, we have enough time during betas |
…Is [streams] (pythonGH-13671) This PR deprecate explicit loop parameters in all public asyncio APIs This issues is split to be easier to review. Second step: streams.py https://bugs.python.org/issue36373
This PR deprecate explicit loop parameters in all public asyncio APIs
This issues is split to be easier to review.
Second step: streams.py
https://bugs.python.org/issue36373