bpo-36889: Document Stream and StreamServer. #14203

tirkarthi · 2019-06-18T15:48:00Z

Add docs for Stream, StreamServer, UnixStreamServer, connect and connect_unix.
Rewrite examples using Stream to avoid DeprecationWarning.

https://bugs.python.org/issue36889

tirkarthi · 2019-06-18T15:56:25Z

Clarifications :

Stream object has mode that is an Enum. Does mode and enum values should be documented? StreamServer and connect that is recommended return Stream with READWRITE value.
Given that StreamReader and StreamWriter are merged together should I copy paste their methods to Stream?
StreamServer and UnixStreamServer have very similar API. I have copy pasted them but does it make sense to just redirect the user with a note to StreamServer that the methods are similar.

Thanks

asvetlov

Good job!
I have several comments/suggestions.

There are connect_read_pipe() and connect_write_pipe() functions. Please document them.
All public StreamServer / UnixStreamServer functions should be documented: bind(), is_bound(), abort().

Doc/library/asyncio-stream.rst

asvetlov · 2019-06-20T13:28:10Z

Doc/library/asyncio-stream.rst

+
+   This function internally uses :meth:`loop.create_connection` to return a
+   :class:`Stream` object of the mode ``READWRITE`` that can be used as a reader
+   and a writer.


Please reflect the fact that async with connect() can be used as well as just await connect().

I have added a note that connect returns a stream object which is awaitable and also about using it as context manager. Let me know if it needs to be reworded that connect itself is awaitable.

Doc/library/asyncio-stream.rst

bedevere-bot · 2019-06-20T13:55:43Z

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…write_pipe and other StreamServer methods

tirkarthi · 2019-06-20T17:00:27Z

Doc/library/asyncio-stream.rst


-        writer.write(query.encode('latin-1'))
+        stream.write(query.encode('latin-1'))
        while True:


Some PEP 572 related example.

while True: line = await stream.readline() if not line: break line = line.decode('latin1').rstrip() if line: print(f'HTTP header> {line}')

while (line := await stream.readline()): line = line.decode('latin1').rstrip() if line: print(f'HTTP header> {line}')

Hmm. Agree, := provides better readable code. Please apply.

tirkarthi · 2019-06-20T17:05:46Z

I have made the requested changes; please review again

Added doc for connect_read_pipe and connect_write_pipe. I have added a note that they can be used as context manager.
Modified all the examples to use context managers.
write calls are awaited.
Changed enter to __aenter__ and exit to __aexit__.
Added all public methods to StreamServer and also updated UnixStreamServer.

Clarifications :

I feel StreamServer and UnixStreamServer are almost exact duplicates. Can UnixStreamServer be added a note that it provides all methods as StreamServer? The downside is that user who visits UnixStreamServer is redirected to StreamServer that there is a slight redirection overhead.
I have documented connect_read_pipe and connect_write_pipe similar that they return Stream object of READ and Write mode. Should we document the StreamMode Enum?

bedevere-bot · 2019-06-20T17:05:49Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

asvetlov

A good step forward!
I left some comments, please fix/discuss

asvetlov · 2019-06-21T07:54:08Z

Doc/library/asyncio-stream.rst

-
-        print('Close the connection')
-        await writer.close()
+            print('Close the connection')


This line is confusing, it is not followed by an actual closing code.
Maybe better to drop it, async with provides enough implication that the stream is closed on exit from the block I guess

Yes, the message about closing is not needed. Removed. Thanks.

asvetlov · 2019-06-21T08:06:20Z

Doc/library/asyncio-stream.rst



+.. coroutinefunction:: connect(host=None, port=None, \*, \
+                               loop=None, limit=2**16, ssl=None, family=0, \


loop parameter is not supported

Thanks, removed.

asvetlov · 2019-06-21T08:10:22Z

Doc/library/asyncio-stream.rst

+
+   This function internally uses :meth:`loop.create_connection` to return an awaitable
+   :class:`Stream` object of the mode ``READWRITE`` that can be used as a reader
+   and a writer. The function can also be used as an async context manager where it's


Just The function can also be used as an async context manager,
drop where it's automatically awaited.
Technically it's not correct: connect returns an internal undocumented object that can be awaited or used by async with. I don't want to document this object but want to provide a clear vision of how to use connect call.

Maybe two short usage examples can help?
E.g.

Connect to TCP socket on *host* : *port* address, return a read-write :class:`Stream` object. The function can be used with ``await`` to get a connected stream:: stream = await asyncio.connect('127.0.0.1', 8888) Also, ``async with`` form is supported, the stream is closed on exit from the block:: async with asyncio.connect('127.0.0.1', 8888) as stream: ... *limit* determines the buffer size limit used by the returned :class:`Stream` instance. By default the limit is set to 64 KiB. The rest of the arguments are passed directly to :meth:`loop.create_connection`.

I'm not sure about the exact text but I hope you've got my idea. Documentation is hard.

Agreed it's hard :) I have adopted your suggestion and formatted it. Did the same for connect_unix too.

asvetlov · 2019-06-21T08:12:20Z

Doc/library/asyncio-stream.rst

+
+.. coroutinefunction:: connect_read_pipe(pipe, *, limit=2**16)
+
+   Takes a :term:`file-like object <file object>` to return a


Suggested change

Takes a :term:`file-like object <file object>` to return a

Takes a :term:`file-like object <file object>` *pipe* to return a

asvetlov · 2019-06-21T08:12:37Z

Doc/library/asyncio-stream.rst

+
+.. coroutinefunction:: connect_write_pipe(pipe, *, limit=2**16)
+
+   Takes a a :term:`file-like object <file object>` to return a


Suggested change

Takes a a :term:`file-like object <file object>` to return a

Takes a a :term:`file-like object <file object>` *pipe* to return a

asvetlov · 2019-06-21T08:16:45Z

Doc/library/asyncio-stream.rst

   .. deprecated-removed:: 3.8 3.10

-      `open_unix_connection()` is deprecated if favor of `connect_unix()`.
+      `open_unix_connection()` is deprecated if favor of :func:`connect_unix`.


Suggested change

`open_unix_connection()` is deprecated if favor of :func:`connect_unix`.

``open_unix_connection()`` is deprecated if favor of :func:`connect_unix`.

asvetlov · 2019-06-21T08:16:59Z

Doc/library/asyncio-stream.rst

   .. deprecated-removed:: 3.8 3.10

-      `start_unix_server()` is deprecated in favor of `UnixStreamServer()`.
+      `start_unix_server()` is deprecated in favor of :class:`UnixStreamServer`.


Suggested change

`start_unix_server()` is deprecated in favor of :class:`UnixStreamServer`.

``start_unix_server()`` is deprecated in favor of :class:`UnixStreamServer`.

asvetlov · 2019-06-21T08:18:57Z

Doc/library/asyncio-stream.rst

+                        limit=2**16, family=socket.AF_UNSPEC, \
+                        flags=socket.AI_PASSIVE, sock=None, backlog=100, \
+                        ssl=None, reuse_address=None, reuse_port=None, \
+                        ssl_handshake_timeout=None, shutdown_timeout=60)


arguments should be described here. A link to loop.create_server() can be used to don't repeat family, flags etc.

Added doc for client_connected_cb and limit. I have added a note that rest of the arguments are passed to loop.create_server.

asvetlov · 2019-06-21T08:19:36Z

Doc/library/asyncio-stream.rst

+
+.. class:: UnixStreamServer(client_connected_cb, /, path=None, *, \
+                            limit=2**16, sock=None, backlog=100, \
+                            ssl=None, ssl_handshake_timeout=None, shutdown_timeout=60)


Describe constructor arguments please

Added doc for client_connected_cb and limit. I have added a note that rest of the arguments are passed to loop.create_unix_server.

asvetlov · 2019-06-21T08:21:12Z

Doc/library/asyncio-stream.rst


-        writer.write(query.encode('latin-1'))
+        stream.write(query.encode('latin-1'))
        while True:


Hmm. Agree, := provides better readable code. Please apply.

asvetlov · 2019-06-21T08:28:27Z

I feel StreamServer and UnixStreamServer are almost exact duplicates. Can UnixStreamServer be added a note that it provides all methods as StreamServer? The downside is that user who visits UnixStreamServer is redirected to StreamServer that there is a slight redirection overhead.

I don't know. I don't want to make their base class public, so duplication is probably ok. We keep references is a good shape at least.

I have documented connect_read_pipe and connect_write_pipe similar that they return Stream object of READ and Write mode. Should we document the StreamMode Enum?

Yes, please do. StreamMode is a public class

tirkarthi · 2019-06-21T17:42:07Z

Changes made :

Documented StreamMode and it's values. Linked the mode values in relevant places like connect, connect_unix, etc.
Used PEP 572 in example for while loop.
Added doc for constructor arguments in StreamServer and UnixStreamServer and a note on rest of the arguments.
Added doc for limit parameter to the functions.
Minor doc fixes to use double backticks and added more links to relevant StreamMode docs.

Please review the changes. Thanks.

asvetlov

Awesome!

miss-islington · 2019-06-24T18:17:02Z

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

(cherry picked from commit 6793cce) Co-authored-by: Xtreak <[email protected]>

bedevere-bot · 2019-06-24T18:17:14Z

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

tirkarthi · 2019-06-24T18:22:18Z

Thank you very much for the guidance Andrew :)

(cherry picked from commit 6793cce) Co-authored-by: Xtreak <[email protected]>

…-14203)" This reverts commit 7268fd0.

tirkarthi added 2 commits June 18, 2019 18:51

Rewrite examples using streams

fee4b78

Document connect, connect_unix, StreamServer and Stream

2d3056c

tirkarthi requested review from 1st1 and asvetlov as code owners June 18, 2019 15:48

bedevere-bot added docs Documentation in the Doc dir awaiting review labels Jun 18, 2019

the-knights-who-say-ni added the CLA signed label Jun 18, 2019

Fix doc errors

29938fc

asvetlov requested changes Jun 20, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jun 20, 2019

Use context manager in examples. Document connect_read_pipe, connect_…

105e983

…write_pipe and other StreamServer methods

tirkarthi commented Jun 20, 2019

View reviewed changes

Use context manager

c16357a

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 20, 2019

asvetlov reviewed Jun 21, 2019

View reviewed changes

Document StreamMode and add more doc for parameters

274f484

asvetlov approved these changes Jun 24, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jun 24, 2019

asvetlov added needs backport to 3.8 skip news labels Jun 24, 2019

asvetlov merged commit 6793cce into python:master Jun 24, 2019

bedevere-bot removed the awaiting merge label Jun 24, 2019

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2019

bpo-36889: Document asyncio Stream and StreamServer (pythonGH-14203)

e984d74

(cherry picked from commit 6793cce) Co-authored-by: Xtreak <[email protected]>

bedevere-bot removed the needs backport to 3.8 label Jun 24, 2019

miss-islington added a commit that referenced this pull request Jun 24, 2019

bpo-36889: Document asyncio Stream and StreamServer (GH-14203)

7268fd0

(cherry picked from commit 6793cce) Co-authored-by: Xtreak <[email protected]>

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-36889: Document asyncio Stream and StreamServer (pythonGH-14203)

23d2f72

aeros added a commit to aeros/cpython that referenced this pull request Sep 27, 2019

Revert "bpo-36889: Document asyncio Stream and StreamServer (pythonGH…

af4687b

…-14203)" This reverts commit 7268fd0.

aeros added a commit to aeros/cpython that referenced this pull request Sep 28, 2019

Revert "bpo-36889: Document asyncio Stream and StreamServer (pythonGH…

cd886e3

…-14203)" This reverts commit 7268fd0.

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-36889: Document asyncio Stream and StreamServer (pythonGH-14203)

d8281ac



		.. coroutinefunction:: connect(host=None, port=None, \*, \
		loop=None, limit=2**16, ssl=None, family=0, \


		.. coroutinefunction:: connect_read_pipe(pipe, , limit=2*16)

		Takes a :term:`file-like object <file object>` to return a


		.. coroutinefunction:: connect_write_pipe(pipe, , limit=2*16)

		Takes a a :term:`file-like object <file object>` to return a

	`open_unix_connection()` is deprecated if favor of :func:`connect_unix`.
	``open_unix_connection()`` is deprecated if favor of :func:`connect_unix`.

	`start_unix_server()` is deprecated in favor of :class:`UnixStreamServer`.
	``start_unix_server()`` is deprecated in favor of :class:`UnixStreamServer`.

Uh oh!

bpo-36889: Document Stream and StreamServer. #14203

bpo-36889: Document Stream and StreamServer. #14203

Uh oh!

Conversation

tirkarthi commented Jun 18, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tirkarthi commented Jun 18, 2019

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tirkarthi commented Jun 20, 2019

Uh oh!

bedevere-bot commented Jun 20, 2019

Uh oh!

asvetlov left a comment • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Jun 21, 2019

Uh oh!

tirkarthi commented Jun 21, 2019

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jun 24, 2019

Uh oh!

bedevere-bot commented Jun 24, 2019

tirkarthi commented Jun 18, 2019 •

edited by bedevere-bot

Loading

asvetlov left a comment •

edited

Loading