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

Fix Mutable Default Arguments in Constructor And Correct Type Hints in Constructor #242

Open
EphraimHaber opened this issue May 17, 2024 · 1 comment · May be fixed by #243
Open

Fix Mutable Default Arguments in Constructor And Correct Type Hints in Constructor #242

EphraimHaber opened this issue May 17, 2024 · 1 comment · May be fixed by #243

Comments

@EphraimHaber
Copy link

The Watchers have some code that can lead to errors.

take LorWatcher.py as an example:

Using mutable objects (e.g., lists, dictionaries) as default arguments for parameters is generally considered a bad practice in Python. In this case, rate_limiter: RateLimiter = BasicRateLimiter() and deserializer: Deserializer = DictionaryDeserializer() could lead to unexpected behavior if you create multiple instances of the class. It's better to use None as the default value and handle the instantiation inside the __init__ method or use a constant or a function to provide the default value.
Type Hints: Using type hints (`api_key: str = None, timeout: int = None) is false and is marked as error.

@pseudonym117
Copy link
Owner

For these specific arguments, this behavior is intentional.

Sharing of a rate limiter between different instances on the same process is intentional. It keeps questionable code-patterns functioning as expected by users, even if it may not be the most pure solution. For example, a naive flask app may create a new LolWatcher instance on each web request. The sharing of the default argument allows this pattern to actually work, and respect the rate limit, despite the fact that the most architecturally sound decision would be the caller storing a singleton instance somewhere in flask.g.

As for the Deserializer, this class is completely stateless. Sharing of a single instance is completely acceptable, and reduces memory pressure in the case where multiple instances are created, as less total objects exist.

So I am inclined to leave these as-is, as changing this would be a breaking change for users, and because I am not convinced that the benefit is there.

As for the type hints - valid. That should be fixed.

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 a pull request may close this issue.

2 participants