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

Refactor split socket and ssl socket #265

Merged

Conversation

betaboon
Copy link
Contributor

continuation of #261 #262 #263 #264

This PR includes the following changes:

  • extend FakeSSLContext by replacing all dummy-methods with full signatures
  • move FakeSSLContext from mocket.ssl to mocket.ssl.context
  • move all mock- and true-functions from mocket.inject to mocket.socket and mocket.ssl.context
  • type all function signatures of MocketSocket
  • split ssl-specific methods from MocketSocket into MocketSSLSocket

Some notes:

socket.read and socket.write seem to have been considered part of the socket-api.
They are infact part of SSLSocket and have thus been incorrectly used.
a test and some smaller internal methods had to be changed accordingly.

@betaboon betaboon marked this pull request as draft November 17, 2024 23:06
@betaboon
Copy link
Contributor Author

betaboon commented Nov 17, 2024

interesting. i tested locally with python3.8, which passes.
with >=3.11 i can reproduce the tests failing. will look at that tomorrow

@mindflayer
Copy link
Owner

mindflayer commented Nov 18, 2024

Hi there, could you please explain why you think this is a good idea?

extend FakeSSLContext by replacing all dummy-methods with full signatures

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 compat module with types comes from).
Libraries can change from a version to another, we don't want to implement full signatures, and that's the reason for having a dummy method used as catch-all. For getter/setter, what is the reason to implement them one by one, if what we do is still return 0 and pass everywhere? Trying to keep Mocket code-base as lean as possible positively impacts its maintainability. And believe me, almost every time there is a new Python version around, there are things to do to make it work.

@betaboon
Copy link
Contributor Author

hey. in my eyes it contributes to readability.

but again, preferences. if you prefer the less-verbose way, i will revert :)

@mindflayer
Copy link
Owner

mindflayer commented Nov 18, 2024

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 Mocket where their code does literally nothing meaningful.

@mindflayer
Copy link
Owner

Also, having a single function helps with debugging.

@betaboon betaboon force-pushed the refactor-split-socket-and-ssl-socket branch from 95e00e0 to 6b7143f Compare November 18, 2024 09:22
@mindflayer
Copy link
Owner

mindflayer commented Nov 18, 2024

socket.read and socket.write seem to have been considered part of the socket-api.
They are infact part of SSLSocket and have thus been incorrectly used.
a test and some smaller internal methods had to be changed accordingly.

Weren't you the one who split the logic into two separated classes: MocketSocket AND MocketSSLSocket?

@betaboon
Copy link
Contributor Author

Weren't you the one who split the logic into two separated classes: MocketSocket AND MocketSSLSocket?

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")

socket.socket does not have a write-method.

@betaboon betaboon force-pushed the refactor-split-socket-and-ssl-socket branch from 6b7143f to d6c1701 Compare November 18, 2024 10:09
@betaboon betaboon force-pushed the refactor-split-socket-and-ssl-socket branch from d6c1701 to 636951f Compare November 18, 2024 10:35
@coveralls
Copy link

Coverage Status

coverage: 99.255% (-0.07%) from 99.324%
when pulling 636951f on betaboon:refactor-split-socket-and-ssl-socket
into 89055e8 on mindflayer:main.

3 similar comments
@coveralls
Copy link

Coverage Status

coverage: 99.255% (-0.07%) from 99.324%
when pulling 636951f on betaboon:refactor-split-socket-and-ssl-socket
into 89055e8 on mindflayer:main.

@coveralls
Copy link

Coverage Status

coverage: 99.255% (-0.07%) from 99.324%
when pulling 636951f on betaboon:refactor-split-socket-and-ssl-socket
into 89055e8 on mindflayer:main.

@coveralls
Copy link

Coverage Status

coverage: 99.255% (-0.07%) from 99.324%
when pulling 636951f on betaboon:refactor-split-socket-and-ssl-socket
into 89055e8 on mindflayer:main.

@mindflayer
Copy link
Owner

yep. exactly for the reason of cleanly separate the apis.

eg this in the tests is just wrong:

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 write(). Good catch!

@betaboon betaboon marked this pull request as ready for review November 18, 2024 10:59
@betaboon
Copy link
Contributor Author

from my perspective this is ready.

@mindflayer
Copy link
Owner

from my perspective this is ready.

I am working atm, I'll review it as soon as possible.

@betaboon
Copy link
Contributor Author

take your time. I'm back at work too :)

Copy link
Owner

@mindflayer mindflayer left a comment

Choose a reason for hiding this comment

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

LGTM!

@mindflayer mindflayer merged commit 8e7b3b6 into mindflayer:main Nov 20, 2024
8 of 9 checks passed
@betaboon betaboon deleted the refactor-split-socket-and-ssl-socket branch November 20, 2024 09:44
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