-
Notifications
You must be signed in to change notification settings - Fork 48
Refactor split socket and ssl socket #265
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
Refactor split socket and ssl socket #265
Conversation
|
interesting. i tested locally with python3.8, which passes. |
|
Hi there, could you please explain why you think this is a good idea?
Mocket is supporting different versions of Python 3. In the past it has even supported Python 2 and 3 at the same time (that's where the |
|
hey. in my eyes it contributes to readability. but again, preferences. if you prefer the less-verbose way, i will revert :) |
|
I don't think that having x lines of copy/pasted functions is more readable than having a single catch-all method, especially in the case of |
|
Also, having a single function helps with debugging. |
95e00e0 to
6b7143f
Compare
Weren't you the one who split the logic into two separated classes: |
yep. exactly for the reason of cleanly separate the apis. eg this in the tests is just wrong: sock = socket.socket()
sock.connect(address[-1])
sock.write(f"{method} {path} HTTP/1.0\r\n")
|
6b7143f to
d6c1701
Compare
d6c1701 to
636951f
Compare
|
3 similar comments
Yes, the test was using the wrong method. It was added by a contributor and I did not spot the fact that it was using |
|
from my perspective this is ready. |
I am working atm, I'll review it as soon as possible. |
|
take your time. I'm back at work too :) |
mindflayer
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!



continuation of #261 #262 #263 #264
This PR includes the following changes:
FakeSSLContextby replacing all dummy-methods with full signaturesFakeSSLContextfrommocket.ssltomocket.ssl.contextmocket.injecttomocket.socketandmocket.ssl.contextMocketSocketMocketSocketintoMocketSSLSocketSome notes:
socket.readandsocket.writeseem to have been considered part of thesocket-api.They are infact part of
SSLSocketand have thus been incorrectly used.a test and some smaller internal methods had to be changed accordingly.