Skip to content

Refactor split socket and ssl socket - #265

Merged
mindflayer merged 9 commits into
mindflayer:mainfrom
betaboon:refactor-split-socket-and-ssl-socket
Nov 20, 2024
Merged

mindflayer merged 9 commits into
mindflayer:mainfrom
betaboon:refactor-split-socket-and-ssl-socket

Conversation

@betaboon

Copy link
Copy Markdown
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

betaboon commented Nov 17, 2024 •

Copy link
Copy Markdown
Contributor Author

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

@mindflayer

mindflayer commented Nov 18, 2024 •

Copy link
Copy Markdown
Owner

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
Copy Markdown
Contributor Author

hey. in my eyes it contributes to readability.

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

@mindflayer

mindflayer commented Nov 18, 2024 •

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

mindflayer commented Nov 18, 2024 •

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
@sonarqubecloud

Copy link
Copy Markdown

@coveralls

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor Author

from my perspective this is ready.

@mindflayer

Copy link
Copy Markdown
Owner

from my perspective this is ready.

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

@betaboon

Copy link
Copy Markdown
Contributor Author

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

@mindflayer mindflayer left a comment

Copy link
Copy Markdown
Owner

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
@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