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. #371

Closed
wants to merge 9 commits into from

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.

As @asvetlov mentioned in #367, the params for the ws_connect method and function may need some fine tuning.

If this is to be merged I will be happy to do docs and doc strings. Thanks in advance for any feedback or suggestions.

(sorry about that extra merge commit)

@@ -167,6 +173,75 @@ def request(self, method, url, *,

return resp

@asyncio.coroutine
def ws_connect(self, url,
protocols=(),
Copy link
Member

Choose a reason for hiding this comment

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

All parameters except url should be keyword-only.

…d to look at ws_connect func params as well
@davebshow
Copy link
Contributor Author

Made the first params fix.

@asvetlov
Copy link
Member

Looks good.

@asvetlov
Copy link
Member

@davebshow do you have something to be done or the PR is ready for merging/final review?

@davebshow
Copy link
Contributor Author

@asvetlov The only thing would be to update the client_reference.rst docs with the new method. I may be able to get to it a bit later tonight. Also there is a comment that I want to remove, will do now. Other than that I think it is ready for final review.

@davebshow
Copy link
Contributor Author

I'm done making changes/adding docs unless there is something else that needs to be changed. Thanks for reviewing @asvetlov!

@davebshow
Copy link
Contributor Author

Build failed when travis couldn't activate python3.3 environment. Should I make another commit and see if it passes? Tried to close and reopen the PR, but then appveyor failed as cancelled.

@davebshow davebshow closed this May 20, 2015
@davebshow davebshow reopened this May 20, 2015
@davebshow davebshow closed this May 20, 2015
@davebshow davebshow reopened this May 20, 2015
@@ -250,8 +271,7 @@ request coroutine
read_until_eof=True, request_class=None,\
response_class=None)

Perform an asynchronous http request. Return a response object
(:class:`ClientResponse` or derived from).
Performs an asynchronous http request. Returns a response object.
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 below changes in this file are side effects of the merge, pollutes the history a bit. Maybe a git rebase will solve this next time. Sorry a bit new the pull ecosystem.

@davebshow
Copy link
Contributor Author

This got a bit messy. I'll submit a clean PR with just my changes.

@davebshow davebshow closed this May 20, 2015
@asvetlov
Copy link
Member

You may just push new code with --force flag.
Github will save conversation history in this case.

@davebshow
Copy link
Contributor Author

Sorry just saw that. I bit of a newb when it comes to PR, moved to #374

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)
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f7b8cdb on davebshow:ws_session into ** on KeepSafe:master**.

@lock
Copy link

lock bot commented Oct 29, 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 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 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.

None yet

3 participants