Skip to content
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

Create docs #71

Merged
merged 4 commits into from
Oct 29, 2018
Merged

Create docs #71

merged 4 commits into from
Oct 29, 2018

Conversation

mehaase
Copy link
Contributor

@mehaase mehaase commented Oct 25, 2018

This PR includes documentation in Sphinx format, published to Read The Docs here:

https://trio-websocket.readthedocs.io/

This should be pretty complete and I've proofread for grammar issues, broken links, etc. It could use a quick review for clarity and omissions.

I've moved some content from README.md to the docs directory and
also greatly expanded the content. I also noticed that I was unwisely
using an Exception where a ValueError would be better, so I changed
that and its corresponding unit test.

After pushing this, I will set up Read The Docs.
* Published to RTD.
* Proofread the previous commit.
* Added sphinxcontrib-trio so that async APIs are documented correctly.
* Added credits page.
@mehaase mehaase requested a review from belm0 October 25, 2018 15:46
@coveralls
Copy link

coveralls commented Oct 25, 2018

Pull Request Test Coverage Report for Build 58

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 91.351%

Totals Coverage Status
Change from base Build 51: 0.3%
Covered Lines: 338
Relevant Lines: 370

💛 - Coveralls

The build fails because it can't find sphinxcontrib_trio, which is
is a "dev" dependency. This quick fix is to make it a real dependency.
This might be fixable by doing #61?
Copy link
Member

@belm0 belm0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks very good

docs/getting_started.rst Outdated Show resolved Hide resolved
docs/clients.rst Show resolved Hide resolved
tasks. If you wish to specify your own nursery instead, you should use the
the following convenience functions instead.

.. autofunction:: connect_websocket
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the API naming: as the nursery for internal tasks is some implementation detail, it seems odd to use "open" vs. "connect" to qualify the function names, which suggests something different is being done with the connection.

Why not use a keyword argument on a common function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The open_* methods are context managers, and the connect_* methods are not.

This was discussed a bit in the client nursery issue. It was also discussed more in Gitter where Nathaniel described a potential use case for supplying a nursery: if the number of connections is determined dynamically, e.g. on one websocket you get a message that says "connect to this other websocket". In an earlier version, I had the nursery version as a keyword argument to a context manager, but this defeats the purpose of spawning new connections into the caller's nursery dynamically. I realize this explanation is a bit cloudy–to be honest I haven't tried writing code like this so it's cloudy in my head, too–but I still think this split between high level and low level APIs is worth keeping.

I don't like the names. The "open" and "connect" prefixes sound similar and don't suggest what might be different about them. I'm open to suggestions here...

includes a convenience function that allows you to wrap any arbitrary Trio
stream with a client WebSocket.

.. autofunction:: wrap_client_stream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why doesn't wrap_client_stream have the nursery option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does have a nursery argument. (The arguments are added by autofunction.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean could it be optional?

As explained in the tutorial, a WebSocket server needs a handler function and a
host/port to bind to. The handler function receives a
:class:`WebSocketRequest` object, and it calls the request's
:func:`~WebSocketRequest.accept` method to finish the handshake and obtain a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you explain what to do if you didn't want to accept the connection, and maybe suggest how the decision would be made (e.g. by resource path?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, there's no way to reject the connection in an RFC compliant way. There are some hacks we could implement, but I'd prefer to wait until wsproto supports this upstream so we can do it right. (Have you looked at python-hyper/wsproto/issues/85?)

docs/servers.rst Show resolved Hide resolved
docs/contributing.rst Show resolved Hide resolved
docs/contributing.rst Show resolved Hide resolved
@mehaase mehaase merged commit 82bd679 into master Oct 29, 2018
@mehaase mehaase deleted the create_docs branch October 29, 2018 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants