Add ws_connect to ClientSession (cleaned up version of pull request #371 - figuring out the right way to do this) #374
Conversation
davebshow
changed the title
| def ws_connect(self, url, *, | ||
| protocols=(), | ||
| timeout=10.0, | ||
| ws_response_class=None, |
There was a problem hiding this comment.
The only possible other change I would make to this patch would be to move ws_response_class to the constructor function to be consistent with ClientSession.request method.
| response_class=None, cookies=None, headers=None,\ | ||
| auth=None) | ||
| response_class=None, ws_response_class=None,\ | ||
| cookies=None, headers=None, auth=None) |
There was a problem hiding this comment.
I noticed here that in the docs it lists the default params: request_class=None, response_class=None (and now ws_response_class=None). However, in the code base these are set to the default classes. Is this an oversight? Or is it intended to be that way?
There was a problem hiding this comment.
Documentation is wrong.
I've changed defaults but forgot to update doc.
Please fix to correct values.
There was a problem hiding this comment.
Got it. These params will not include the (optional) flag correct? Because some class must be passed as arg for each (although it is usually the default) . Like such:
:param request_class: Request class implementation. ClientRequest
by default.
:param response_class: Response class implementation.
ClientResponse by default.
:param ws_response_class: WebSocketResponse class implementation.
ClientWebSocketResponse by default.
…aram order so it looks better
|
This patch should be ready for review after the appveyor CI completes. Unless there is something else to update/change... |
| def ws_connect(url, protocols=(), timeout=10.0, connector=None, | ||
| response_class=None, autoclose=True, autoping=True, loop=None): | ||
| """Initiate websocket connection.""" | ||
| def ws_connect(url, *, protocols=(), timeout=10.0, connector=None, |
There was a problem hiding this comment.
Please add @asyncio.coroutine decorator.
|
The code is pretty good. |
|
Made all the additions. |
Add ws_connect to ClientSession (cleaned up version of pull request #371 - figuring out the right way to do this)
|
Perfect! Thanks! |
Added ClientSession.ws_connect method. Pretty much a copy paste of old websocket_client.ws_connect. The websocket_client.ws_connect now is very similar to client.request in that it creates a ClientSession with a TCPConnector(force_close=True) and calls that session's ws_connect method.
Had to change the mocks in the websocket client tests a bit.