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: introduce state object #264

Merged
merged 2 commits into from
Nov 17, 2024

Conversation

betaboon
Copy link
Contributor

@betaboon betaboon commented Nov 17, 2024

continuation of #261 and #263

This PR does two things:

  • move the enable- and disable-logic into a dedicated file
  • introduce a state-object to replace the singleton-class

This is done to prevent cyclic-imports.

@coveralls
Copy link

coveralls commented Nov 17, 2024

Coverage Status

coverage: 99.324% (-0.2%) from 99.545%
when pulling 6291c31 on betaboon:refactor-introduce-state-object
into 2beb8f1 on mindflayer:main.

@mindflayer
Copy link
Owner

As I was previously commenting, I don't particularly like this Mocket -> MocketState change. It's not a MocketState we are dealing with, it's Mocket we enable/disable.
I believe it makes things more difficult to write, and expose internals that IMHO should not be exposed.

@mindflayer
Copy link
Owner

OK, now I see there is more. Let me think about that. Thanks!

@mindflayer
Copy link
Owner

mindflayer commented Nov 17, 2024

While I like the idea of moving the two monkey-patch functions to a different module, I don't think that this mocket.state.state is more readable or understandable than the previous Singleton.

@betaboon
Copy link
Contributor Author

just so i understand, before coming up with another approach:

are you mostly concerned about about the callsites (ie the mocket.socket.socket vs Mocket. ?

if so, how about this pattern instead:

from mocket.state import Mocket

def some_function() -> None:
     Mocket.register()

by just doing the following in mocket/state.py:

Mocket = MocketState()

?

@mindflayer
Copy link
Owner

mindflayer commented Nov 17, 2024

Let's split into two different aspects:

  • First: what you calling MocketState to me is simply Mocket, it's not its state (this is philosophy, I understand you may have a different point of view);
  • Second: what is the reason to convert the Singleton in class/instance when Mocket API is designed to work as a Singleton? If something works like a Singleton, for the sake of readability, isn't it better if it's also implemented as a Singleton?

@mindflayer
Copy link
Owner

mindflayer commented Nov 17, 2024

  • First: what you calling MocketState to me is simply Mocket, it's not its state (this is philosophy, I understand you may have a different point of view);

Let me rephrase it: finding a class called MocketState is simply confusing, and I don't think it'd be beneficial to the users that already know Mocket. So, this concerns is mostly about "naming", and I guess we both know that naming is really an important - and delicate - part of software development.

@betaboon
Copy link
Contributor Author

i think I just stepped into the age-old trap of singleton patterns in python.

https://discuss.python.org/t/any-extra-reason-to-not-use-classes-for-singletons/19243/5

where there are two options:

  1. have a class bug create a single instance of it
  2. have a class that exposes everything through classmethods, that never get's instantiated

you seem to prefer option2, for understandable reasons, while i prefer option 1.
i guess both are valid (aside from you good point against calling it state).

I'll try to apply option2 :)

@mindflayer
Copy link
Owner

mindflayer commented Nov 17, 2024

Honestly: I am not sure I would write it this way today. We did it in 2013 and I don't see any strong reason to change it today.
I can tell you more, I don't normally write Singletons in Python, there is a better way to write something similar called "Borg". I see it's also mentioned in the thread you shared.

@betaboon
Copy link
Contributor Author

yeah. i also rarely use singletons.
but 🤷 the world is full of learning and preferences :)

@mindflayer
Copy link
Owner

The thing here is: we don't need to instantiate Mocket more than once, expecting we always get back the same instance. So, it's not even used as a Singleton, it's just a class implementing some methods, it's pure syntactic sugar.

@betaboon betaboon marked this pull request as draft November 17, 2024 18:55
refactor: Mocket - add typing and get rid of cyclic import
@betaboon betaboon force-pushed the refactor-introduce-state-object branch from 5833f65 to 6291c31 Compare November 17, 2024 18:59
@betaboon
Copy link
Contributor Author

ok it's back to a singleton-class, with the injection-logic extracted.

i had to change one import mocket.mocket.MocketSocket which would lead to cyclic imports.

@betaboon betaboon marked this pull request as ready for review November 17, 2024 19:27
@mindflayer
Copy link
Owner

ok it's back to a singleton-class, with the injection-logic extracted.

I really appreciate your patience. :)

@betaboon
Copy link
Contributor Author

of course. you're the maintainer, I'm just a guy.

opensource lives from open, friendly and respectful collaboration :)

@mindflayer mindflayer merged commit 89055e8 into mindflayer:main Nov 17, 2024
8 of 9 checks passed
@betaboon betaboon deleted the refactor-introduce-state-object branch November 17, 2024 19:34
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