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

support new param 'response_class' in websocket_client.ws_connect. #367

Merged
merged 5 commits into from
May 17, 2015

Conversation

davebshow
Copy link
Contributor

Adds a 'response_class' parameter to websocket_client.ws_connect that allows user to implement a custom ClientWebSocketResponse style class and pass it to the ws_connect coroutine. Similar to 'response_class' param for aiohttp.client.request.

@@ -87,7 +87,9 @@ def ws_connect(url, protocols=(), timeout=10.0, connector=None,
reader = resp.connection.reader.set_parser(WebSocketParser)
writer = WebSocketWriter(resp.connection.writer, use_mask=True)

return ClientWebSocketResponse(
response_class = response_class or ClientWebSocketResponse
Copy link
Member

Choose a reason for hiding this comment

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

Please use

if response_class is None:
    response_class = ClientWebSocketResponse idiom

@asvetlov
Copy link
Member

The patch is LGTM (except my inline note).
I think we should add websockets (and maybe multiparts) to ClientSession also.

@davebshow
Copy link
Contributor Author

Thanks asvetlov! I updated the patch to reflect your inline note.

@davebshow
Copy link
Contributor Author

Couple more small changes for consistency in the docs. I'm done now with this feature.

asvetlov added a commit that referenced this pull request May 17, 2015
support new param 'response_class' in websocket_client.ws_connect.
@asvetlov asvetlov merged commit cad2ad6 into aio-libs:master May 17, 2015
@asvetlov
Copy link
Member

Thanks!

@davebshow davebshow deleted the ws_response_class branch May 17, 2015 21:24
@davebshow
Copy link
Contributor Author

Thank you for reviewing @asvetlov Quick question: when you said earlier "I think we should add websockets (and maybe multiparts) to ClientSession also", were you referring to using the ClientSession to issue the handshake request and in this way establishing multiple websocket connections using the same ClientSession? If so, I was already implementing something like this in a project using aiohttp components. It would be easy to submit a patch. Maybe I'm off though and you were thinking of something a bit more hardcore.

@asvetlov
Copy link
Member

I think we need ClientSession.connect_ws() coroutine, method parameters may be subject of discussion.

@asvetlov
Copy link
Member

@davebshow if you would make PR for that -- please do. It will be at least nice starting point.

@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants