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

Add ws_connect to ClientSession (cleaned up version of pull request #371 - figuring out the right way to do this) #374

Merged
merged 5 commits into from
May 22, 2015

Conversation

davebshow
Copy link
Contributor

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.

@davebshow davebshow changed the title Cleaned up version of pull request #371 (figuring out the right way to do this) Add ws_connect to ClientSession (cleaned up version of pull request #371 - figuring out the right way to do this) May 20, 2015
def ws_connect(self, url, *,
protocols=(),
timeout=10.0,
ws_response_class=None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Agree. Please do.

response_class=None, cookies=None, headers=None,\
auth=None)
response_class=None, ws_response_class=None,\
cookies=None, headers=None, auth=None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Documentation is wrong.
I've changed defaults but forgot to update doc.
Please fix to correct values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@davebshow
Copy link
Contributor Author

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,
Copy link
Member

Choose a reason for hiding this comment

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

Please add @asyncio.coroutine decorator.

@asvetlov
Copy link
Member

The code is pretty good.
Please fix my comments -- I'll merge PR after that.

@davebshow
Copy link
Contributor Author

Made all the additions.

asvetlov added a commit that referenced this pull request May 22, 2015
Add ws_connect to ClientSession (cleaned up version of pull request #371 - figuring out the right way to do this)
@asvetlov asvetlov merged commit 5f3605a into aio-libs:master May 22, 2015
@asvetlov
Copy link
Member

Perfect! Thanks!

@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