Skip to content
This repository has been archived by the owner on Jan 22, 2024. It is now read-only.

Make connection handling more explicit and remove outdated examples #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zvyn
Copy link
Contributor

@zvyn zvyn commented Feb 21, 2023

With this patch we can be more certain that the connection is not kept
for longer than the read_timeout.

zvyn added 3 commits February 21, 2023 20:08
With this patch we can be more certain that the connection is not kept
for longer than the `read_timeout`.
Refer to https://developer.geops.io for up to date documentation.
@zvyn zvyn requested a review from aheld84 February 21, 2023 19:30
Copy link
Member

@aheld84 aheld84 left a comment

Choose a reason for hiding this comment

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

There are two points that need clarification. Otherwise it looks good.

redis_websocket_api/handler.py Outdated Show resolved Hide resolved
redis_websocket_api/server.py Outdated Show resolved Hide resolved
This deprecates the `read_timeout` argument but does not brake the
existing public API.

Also switched some early Python 3.5 asyncio coding style for Python 3.7+
patterns. Most notable the `loop` is only used directly if given as
parameter.
@zvyn zvyn requested a review from aheld84 March 6, 2023 10:19
Copy link
Member

@aheld84 aheld84 left a comment

Choose a reason for hiding this comment

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

I have one question and some suggestions for typos. Otherwise it looks good.

redis_websocket_api/handler.py Outdated Show resolved Hide resolved
redis_websocket_api/handler.py Outdated Show resolved Hide resolved
redis_websocket_api/server.py Outdated Show resolved Hide resolved
redis_websocket_api/server.py Outdated Show resolved Hide resolved
@zvyn zvyn requested a review from aheld84 March 6, 2023 16:28
Copy link
Member

@aheld84 aheld84 left a comment

Choose a reason for hiding this comment

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

Ready to be merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants