Refactoring and Typing

[betaboon]

No description provided.

[mindflayer]

Hi @betaboon, you decided to move a lot of code to dedicated modules. While I understand the reasons behind, and I see it as valuable, I would keep it as a complete different task.

The way it is now complicates understanding what's changing because of type hints and what is just code that got moved to a different file.

[betaboon]

@mindflayer I'm trying to keep those steps in individual commits so they are easier to follow.

i hope looking at changes commit-by-commit makes more sense.

do you have any suggestions how to approach this differently?

[mindflayer]

For a PR that is meant to introduce type hints is probably too much. I'd keep them as two completely separated tasks, and avoid breaking interfaces (names, import paths, etc).
For instance: I see you renamed a class, and I would not do it, at least without leaving the old name where it was.

[mindflayer]

When we have type hints in place, and we are sure that nothing broke, we can start discussing about introducing new modules (and how they should be called).

[mindflayer]

Imagine yourself as a Mocket user, with a broken CI, trying to understand what happened, while staring at a "thousand lines changed" PR which was only supposed to add type hints (I am not saying "only" because I see it as a simple task).

[betaboon]

for clarity: Sorry for stomping in, I didn't mean to intrude or offend anyone.

this PR wasn't really intended to get merged as a whole, but rather to play around with approaches to get this typed and show you what I'm thinking.

having poked around the codebase i believe refactoring is required to get it into a state where it can be fully typed.

If you're interested in my contributions to get there I'd like to send individual PRs for the steps that i have in mind.

Every step must keep the tests green and the API backwards-compatible.
Every step will lead to more complete typing where possible.

These are the steps i have in mind:

1. make imports absolute
2. remove the compat-types
3. introduce the state-object
4. split mocket.py into separate modules
5. rename the FakeSSLContext (to MocketSSLContext) and MocketSocketCore (to MocketSocketIO) to create naming-symmetry with the stdlib socket and ssl libraries
6. introduce a class for storing and retrieving recordings
7. split MocketSocket cleanly into MocketSocket and MocketSocketSSL
8. refactor the Entry-, Request- and Response-classes to use proper subclassing

I'd be quite interested to hear what you're thinking.

Thanks for your patience and understanding

PS: i just reset this branch to only contain 1. and 2. and moved my other wip-stuff into a separate branch.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[mindflayer]

No offence taken, and let me add you are the first contributor that is not just interested - which is still legit and welcome - in having something fixed or added.

No problems with 1/2/3, it just makes sense.
Points 4 and 5: I's fine as long as we don't break imports. Please note I am not only referring to breaking tests. If things get moved/renamed, which is fine, we need to leave place-holders in place for retro-compatibility.

Point 6: yes please! :) Being wanting to make it better for ages.

Point 7: let's keep the main class as it is and implement mixins instead. Not sure, but maybe that's exactly what you had in mind.

Point 8: I see it more as something better solved by creating interfaces than a problem solved by standard sub-classing.

What do you think?

[mindflayer]

If you're interested in my contributions to get there I'd like to send individual PRs for the steps that i have in mind.

Every step must keep the tests green and the API backwards-compatible.

I am more than happy to see this happening, if my previous answer what not clear enough.

And thank you, btw. :)

[betaboon]

Points 4 and 5: I's fine as long as we don't break imports. Please note I am not only referring to breaking tests. If things get moved/renamed, which is fine, we need to leave place-holders in place for retro-compatibility.

i agree. keeping the old names alive, until they can get explicitly deprecated.

Point 7: let's keep the main class as it is and implement mixins instead. Not sure, but maybe that's exactly what you had in mind.

I'm actually thinking about MocketSSLSocket(MocketSocket), as to isolate the ssl and non-ssl apis.

so basically MocketSSLContext.wrap_socket(sock: MocketSocket) -> MocketSSLSocket

Point 8: I see it more as something better solved by creating interfaces than a problem solved by standard sub-classing.

Yeah i'm thinking off abc.ABC atleast for Entry.

i think for responses it would actually be better to do HTTPResponse(BytesResponse)

[mindflayer]

so basically MocketSSLContext.wrap_socket(sock: MocketSocket) -> MocketSSLSocket

I don't think there is a specific need for having a MocketSSLSocket, since what is returned is always a simple MocketSocket. We fake all the faff about SSL certificates, and then give back a normal MocketSocket.

[betaboon]

so basically MocketSSLContext.wrap_socket(sock: MocketSocket) -> MocketSSLSocket

I don't think there is a specific need for having a MocketSSLSocket, since what is returned is always a simple MocketSocket. We fake all the faff about SSL certificates, and then give back a normal MocketSocket.

i agree that there is not specific need, but tbh the MocketSocket class currently provides three different interfaces, socket.socket + ssl.SSLSocket + (not sure about that yet) BytesIO.

i'd prefer having that cleanly split, so that you can actually know what is part of which api.

[mindflayer]

Mocket is basically mocking all the entrypoints available to create a socket.socket, with the only objective of returning a MocketSocket instead. Everything under the hood is only there for supporting different client behaviours: blocking vs non-blocking sockets, etc.

[betaboon]

i'm just trying to introduce the state-object, but i have a feeling i need to do split mocket.py into separate modules first.

otherwise I keep struggling with cyclical imports

[betaboon]

@mindflayer what do you think about the 2 commits in this PR?
they cover 1. and 2. and it would be nice to get them merged.

should i move them into a separate/new PR, to keep this pr open for discussion?

[betaboon]

the commits addressing 1 and 2 (convert imports to absolute and remove the compat-type) are in this branch:
main...betaboon:python-mocket:refactor-absolute-imports-and-remove-compat

the commits addressing 4 (split mocket.py into multiple modules) are on this branch:
betaboon/python-mocket@refactor-absolute-imports-and-remove-compat...betaboon:python-mocket:refactor-split-modules

the commits addressing 3 (introduce a state-object) are on this branch:
betaboon/python-mocket@refactor-split-modules...betaboon:python-mocket:refactor-introduce-state-object

[mindflayer]

the commits addressing 1 and 2 (convert imports to absolute and remove the compat-type) are in this branch:

I've just merged those two.

[betaboon]

ok. I'll create the next PR then :)

[mindflayer]

Now that I spent more time looking into it, I don't particularly like this Mocket -> MocketState change.
You are enabling/disabling Mocket, so I'd rather keep it the way it was. What was the need for it?

[betaboon]

the Mocket class is used as a global singleton to hold state using class-variables.

as the state is set during import-time, this leads to cyclic-imports between eg Mocketand MocketEntry (and almost all others)

moving this out into a class and the instatiating the state once, helps getting rid of the cyclic-imports.

[betaboon]

@mindflayer
I'm currently working on introducing a class for storing and retrieving records.

It's quite hard to get this backwards compatible.
both in api (somewhat possible) and storage format.

how set are you on keeping that part backwards compatible?

[mindflayer]

how set are you on keeping that part backwards compatible?

This would mean that every test suite relying on recorded sessions would break (producing new records).
I don't think that's a good idea.

What are your struggles?

[betaboon]

multiple aspects:

• the json-file is being written on every request, instead of during disable
• the fallback to md5-hashes while looking-up records here
• the code saying that json.dumps should do indent=4, but the json-files in the tests all being indent=2

[mindflayer]

the json-file is being written on every request, instead of during disable

Why would this be a problem? It's definitely better in terms of memory management, since Mocket does not have to remember everything before dumping it on file.

The other two points could be solved without impacting on backwards compatibility.

This said, I was already considering refactoring that part of Mocket, I could take it.

[mindflayer]

Also: the fact that the JSON mocks are dumped with 2 spaces shows that Mocket is agnostic to the formatting, which to me sounds like a pros.

[betaboon]

Why would this be a problem? It's definitely better in terms of memory management, since Mocket does not have to remember everything before dumping it on file.

imho doing json-serialization and filesystem-io (twice) on every request is a bigger downside than keeping the requests in memory until it is being written.

Also: the fact that the JSON mocks are dumped with 2 spaces shows that Mocket is agnostic to the formatting, which to me sounds like a pros.

i was just a little confused that the code is supposed do do 4-indent, but the files in the tests are 2-indent.

i'll tinker around some more to find ways to keep this backwards-compatible.

just thought it couldn't hurt asking your stance first :)