bpo-38066: Hide internal Stream methods by asvetlov · Pull Request #15762 · python/cpython

asvetlov · 2019-09-09T11:41:36Z

feed_eof(), feed_data(), set_exception(), and set_transport() are prefixed with underscore now.

https://bugs.python.org/issue38066

Automerge-Triggered-By: @asvetlov

1st1 · 2019-09-09T13:06:00Z

@asvetlov Andrew, can we also remove the Stream.transport property?

asvetlov · 2019-09-09T13:25:56Z

I think we can but it makes subprocess API a little less backward compatible.
Really wonder how much code is broken.
I'm working on backward-compatible approach: keep all mentioned method plus transport with underscore-prefixed version for internal usage and public version for backward compatibility that raises DeprecationWarding. It is the safest approach that we can do.

1st1 · 2019-09-09T13:54:23Z

But asyncio.Stream is a new thing - what backward compatibility problems do we have with it?

asvetlov · 2019-09-09T13:58:32Z

create_subprocess_exec / create_subprocess_exec used to work with StreamReader/StreamWriter, now they use asyncio.Stream. The replacement is drop-in, Stream has all API methods from reader and writer.

1st1 · 2019-09-09T14:02:03Z

Ah, right! Then your approach with deprecating them gracefully works. Don't forget to also update the docs.

asvetlov · 2019-09-09T18:55:30Z

Legacy methods are restored with emitting deprecation warnings.

asvetlov · 2019-09-09T18:56:51Z


    @property
    def transport(self):
+        warnings.warn("Stream.transport attribute is deprecated "


asyncio itself never uses this property, there is no need for readonly internal API version

asvetlov · 2019-09-09T19:03:21Z

        # The helper tries to use fast-path to return already existing
        # complete future object if underlying transport is not paused
-        #and actual waiting for writing resume is not needed
+        # and actual waiting for writing resume is not needed


I think it's ok to fix this unrelated comment spacing here. Don't worth a separate PR

aeros

Thanks for the PR @asvetlov.

I have a couple of minor grammar suggestions.

aeros · 2019-09-10T08:51:57Z

@@ -1366,7 +1371,7 @@ def writelines(self, data):
    def _fast_drain(self):
        # The helper tries to use fast-path to return already existing
        # complete future object if underlying transport is not paused


Unrelated to the PR, but could make a minor grammar fix here? I could make further suggestions to the phrasing of the code comments for this method, but I figured this suggestion would be easier and uncontroversial.

Suggested change

# complete future object if underlying transport is not paused

# complete future object if the underlying transport is not paused

fast drain works with the single object, not multiple ones.

fast drain works with the single object, not multiple ones.

Oops, I'll adjust the suggestion.

1st1 · 2019-09-10T12:48:04Z

Great!

miss-islington · 2019-09-10T12:56:17Z

Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

feed_eof(), feed_data(), set_exception(), and set_transport() are prefixed with underscore now. https://bugs.python.org/issue38066 (cherry picked from commit 12c122a) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>

bedevere-bot · 2019-09-10T12:56:26Z

GH-15847 is a backport of this pull request to the 3.8 branch.

feed_eof(), feed_data(), set_exception(), and set_transport() are prefixed with underscore now. https://bugs.python.org/issue38066 (cherry picked from commit 12c122a) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>

feed_eof(), feed_data(), set_exception(), and set_transport() are prefixed with underscore now. https://bugs.python.org/issue38066

bpo-38066: Hide internal Stream methods

90bed4a

asvetlov added topic-asyncio needs backport to 3.8 labels Sep 9, 2019

asvetlov requested review from 1st1 and gpshead as code owners September 9, 2019 11:41

the-knights-who-say-ni added the CLA signed label Sep 9, 2019

bedevere-bot added the awaiting core review label Sep 9, 2019

1st1 approved these changes Sep 9, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 9, 2019

gpshead removed their request for review September 9, 2019 12:44

gpshead assigned 1st1 Sep 9, 2019

Deprecate legacy methods

341bc82

asvetlov commented Sep 9, 2019

View reviewed changes

Restore missing .set_exception()

fb28c62

asvetlov commented Sep 9, 2019

View reviewed changes

asvetlov mentioned this pull request Sep 9, 2019

bpo-36889: Document Stream class and add docstrings #14488

Merged

aeros reviewed Sep 10, 2019

View reviewed changes

Fix grammar

8d46f6b

1st1 approved these changes Sep 10, 2019

View reviewed changes

asvetlov added the 🤖 automerge label Sep 10, 2019

miss-islington merged commit 12c122a into python:master Sep 10, 2019

bedevere-bot removed the awaiting merge label Sep 10, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 10, 2019

asvetlov deleted the hide-private-streams-api branch September 10, 2019 12:56

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020

bpo-38066: Hide internal Stream methods (pythonGH-15762)

a99d0b6

feed_eof(), feed_data(), set_exception(), and set_transport() are prefixed with underscore now. https://bugs.python.org/issue38066

	# complete future object if underlying transport is not paused
	# complete future object if the underlying transport is not paused

Uh oh!

Uh oh!

Conversation

asvetlov commented Sep 9, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Sep 9, 2019

Uh oh!

asvetlov commented Sep 9, 2019

Uh oh!

1st1 commented Sep 9, 2019

Uh oh!

asvetlov commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Sep 9, 2019

Uh oh!

asvetlov commented Sep 9, 2019

Uh oh!

asvetlov Sep 9, 2019 • 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!

asvetlov Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeros Sep 10, 2019 • 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!

asvetlov Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

aeros Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

1st1 commented Sep 10, 2019

Uh oh!

miss-islington commented Sep 10, 2019

Uh oh!

bedevere-bot commented Sep 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asvetlov commented Sep 9, 2019 •

edited by miss-islington

Loading

asvetlov commented Sep 9, 2019 •

edited

Loading

asvetlov Sep 9, 2019 •

edited

Loading

aeros Sep 10, 2019 •

edited

Loading