Skip to content

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

Merged
asvetlov merged 5 commits intoaio-libs:masterfrom
davebshow:ws_response_class
May 17, 2015
Merged

support new param 'response_class' in websocket_client.ws_connect.#367
asvetlov merged 5 commits intoaio-libs:masterfrom
davebshow:ws_response_class

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.

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