-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add close method to websocket client #146
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
Conversation
b0bf814
to
94861c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions...
""" | ||
close websocket connection. | ||
""" | ||
self._connected = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check that the socket is open before closing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
close method is safe to call even if the socket is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks.
@@ -191,6 +191,14 @@ def run_forever(self, timeout=None): | |||
while self.is_open(): | |||
self.update(timeout=None) | |||
|
|||
def close(self, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are some of the variable arguments you envision being used here? Just trying to get a feel for this change...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just out of curiosity, should we return True/False if we were able to close successfully the socket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at WebScoket close method for more information: https://github.com/websocket-client/websocket-client/blob/master/websocket/_core.py#L372
that method does not return any status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks.
fixes #145