-
Notifications
You must be signed in to change notification settings - Fork 5
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
Rename checkConnection #20
Conversation
@@ -559,10 +559,15 @@ def do_disconnect(self, _args): | |||
self.commThread.stop() | |||
self.commThread = None | |||
|
|||
def checkConnection(self): | |||
if self.commThread is None or not self.commThread.checkConnection(): | |||
def connection_established(self) -> bool: |
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.
shouldn't we name this different. I see that we had aslo two checkConnection functions but I kind of find it confusing.
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.
Do you have a better suggestion on method name?
If we look at actual behavior the implementation (for both ws and grpc) that a variable is set to true when we succeed with the initial connection, then it remains true until we request the connection to be closed. If ther server/broker goes down does not affect what the method return.
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.
Ping @lukasmittag
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.
hmm, had another look now and I don't know why I found it confusing back then :) so think its fine
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.
else LGTM
@@ -559,10 +559,15 @@ def do_disconnect(self, _args): | |||
self.commThread.stop() | |||
self.commThread = None | |||
|
|||
def checkConnection(self): | |||
if self.commThread is None or not self.commThread.checkConnection(): | |||
def connection_established(self) -> bool: |
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.
hmm, had another look now and I don't know why I found it confusing back then :) so think its fine
The previous method name could give the impression that the connection actually would be checked when the method was called. What the method actually does it just to report the flag that is set when the initial connection has been established and the client is ready to receive data from the broker/server. That should hopefully be more clear after the change of name. Old name kept on "top level" to not introduce any backward incompatible changes. Fixes eclipse/kuksa.val#523
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.
looks good after rebase
The previous method name could give the impression that the connection actually would be checked when the method was called.
What the method actually does it just to report the flag that is set when the initial connection has been established and the client is ready to receive data from the broker/server. That should hopefully be more clear after the change of name.
Old name kept on "top level" to not introduce any backward incompatible changes.
Fixes eclipse/kuksa.val#523