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
Enhancement ssl websocket #6
Conversation
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.
Small changes. Make SSL available on more ports. Is it worth supporting http/https as protocol names?
@@ -58,7 +66,7 @@ class Websocket: | |||
is_client = False | |||
|
|||
def __init__(self, sock): | |||
self._sock = sock | |||
self.sock = sock |
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.
Why change this?
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.
The reason I did this is that I was trying to port to the ESP32 a project that uses websocket-client and in this case the client has sock
as attribute. Using the same attribute name makes it more compatible.
If that's the prefered way, I can revert to _sock
and modify my port of the other library instead. Please let me know which is best.
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.
Okay that's a good reason.
Thank you for the feedback. I replied to the comments individually and posted an updated version. |
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.
Looking good. One more bit of feedback.
uwebsockets/protocol.py
Outdated
host = match.group(2) | ||
port = match.group(3) | ||
path = match.group(4) | ||
protocol, host, port, path = [match.group(i) for i in range(1, 5)] |
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.
range
is weird here. Just use the group names I think. It's more lucid.
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.
Fixed that.
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.
One line was too long, so I just put it back on separate lines.
Thanks. I added one extra small change which is to close the websockets at the end of |
@danni , ping 😄 Any chance you had time to review the changes ? Cheers! |
Sorry, I missed the notification for this. Done! Thanks! |
No worries, thanks a lot! |
Hi, thanks for a great project!
I have added basic support to connect to secure websockets via the
ussl
module. There were also some minor changes to make here and there because of different api for ssl sockets compared to regular sockets (send
andrecv
are not available andread
andwrite
need to be used instead).I have also added commands in the
Makefile
to create the sub-directories and also clean up everything.I'd be grateful if you review the code and let me know of fixes and modifications that would allow it to be merged.
Cheers!