-
Notifications
You must be signed in to change notification settings - Fork 687
Separate the debugger-client to support the later usage of other communication protocols #2764
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
Separate the debugger-client to support the later usage of other communication protocols #2764
Conversation
zherczeg
left a comment
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.
Nice patch! Only one minor comment fix.
jerry-debugger/jerry_client_tcp.py
Outdated
| return self.socket.send(data) | ||
|
|
||
| def ready(self): | ||
| """ Monitor the file descriptor, return weither it became ready for the I/O operations or not. """ |
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.
I think this should only be used with receive_data, so the comment is not exactly right.
jerry-debugger/jerry_client_tcp.py
Outdated
| if self.socket not in result: | ||
| return False | ||
|
|
||
| return True |
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.
I think you can use return self.socket in result here.
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.
Indeed, good catch. :)
…unication protocols JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
5ec6ecb to
d3e01be
Compare
rerobika
left a comment
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.
LGTM (informal)
zherczeg
left a comment
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.
LGTM
LaszloLango
left a comment
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.
LGTM
| WEBSOCKET_FIN_BIT = 0x80 | ||
|
|
||
| class WebSocket(object): | ||
| def __init__(self, address, protocol=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.
Using the Socket() as default argument is not correct. The protocol will have a reference to the created object and if the WebSocket is constructed multiple times the same socket will be used in both cases.
example.:
a = WebSocket()
b = WebSocket()
a.protocol == b.protocol
We should use None as default and add an check in the constructor to create the socket object.
| self.data_buffer += self.protocol.receive_data() | ||
|
|
||
| if self.data_buffer[0:len_expected] != expected: | ||
| raise Exception("Unexpected handshake") |
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.
In the future it would be good to have specialized Exception classes so we can catch our module's exceptions in a uniform fashion and would make it possible to distinguish between the "base" exceptions ans our exceptions.
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.
This applies to all exceptions in this PR.
No description provided.