Implement timeouts (#64) #89
Conversation
This includes tests and docs.
Pull Request Test Coverage Report for Build 98
💛 - Coveralls |
|
I forgot to mention that I modified the connection logging a bit, it now prints out |
docs/timeouts.rst
Outdated
|
|
||
| .. note:: | ||
|
|
||
| The built-in timeouts do not affect the contents of the block! In this |
There was a problem hiding this comment.
maybe "not affect websocket messaging within the context manger scope"
There was a problem hiding this comment.
Are you objecting to the phrase "affect the contents" or the term "block"?
I think the former is accurate, in the sense that nothing you do inside that block will be affected by these timeout arguments, i.e. even if you trio.sleep(math.inf) that wouldn't time out. It would be overly narrow to say that the timeouts don't affect messaging.
As for "block", I'm uncertain. I originally wrote something more verbose like "context manager block" but it felt unwieldy to keep using that phrase. Maybe spell it out once and then abbreviate it as CM block?
docs/timeouts.rst
Outdated
| you may disable them or use lower-level APIs if you want more control over the | ||
| behavior. | ||
|
|
||
| Built-in Client Timeouts |
There was a problem hiding this comment.
I prefer only capitalizing the first word in headings: less of a distracting artifact
There was a problem hiding this comment.
I'm not opposed to the style change, but all of the other documentation has headings that are capitalized like this.
docs/timeouts.rst
Outdated
| message, then the client will block indefinitely on ``ws.get_message()``. | ||
| Placing timeouts inside blocks is discussed below. | ||
|
|
||
| What if you decided that you really wanted to manage the timeouts yourself? The |
There was a problem hiding this comment.
Perhaps open with this example, as motivation for adding timeouts to the API, and to make it clear what the API timeouts are doing under the hood?
This includes tests and docs.
I also started thinking about how to make tests more deterministic, i.e. #88. I don't have a full solution to that here, but I did write these new tests more carefully and included some comments on the test module that should guide additional effort to reduce race conditions in the test.