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

[+] WebsocketClient thread naming #351

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[+] WebsocketClient thread naming #351

wants to merge 3 commits into from

Conversation

nazariyv
Copy link

  1. Added kwarg for the name of the main thread of WSC
  2. Added kwarg for the keepalive thread name

1. Added kwarg in the constructor of the WSC to name the thread on which it runs
2. Added kwarfg for the keepalive thread.
@TristanJM
Copy link

This will be really useful for debugging
Good PR

@nazariyv
Copy link
Author

Hey, can we merge this, please?

Copy link
Collaborator

@mcardillo55 mcardillo55 left a comment

Choose a reason for hiding this comment

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

Hi! I like the idea for logging/debugging purposes, but I noticed a couple syntax issues. Thanks!

@@ -43,8 +44,8 @@ def _go():

self.stop = False
self.on_open()
self.thread = Thread(target=_go)
self.keepalive = Thread(target=self._keepalive)
self.thread = Thread(target=_go, name=thread_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should reference self.thread_name instead.

self.thread = Thread(target=_go)
self.keepalive = Thread(target=self._keepalive)
self.thread = Thread(target=_go, name=thread_name)
self.keepalive = Thread(target=self._keepalive, name=keeaplive_thread_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should reference self.keepalive_thread_name. Also there is a typo in "keepalive"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants