pseudonym117 / Riot-Watcher

Simple Python wrapper for the Riot Games API for League of Legends
MIT License
531 stars 150 forks source link

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

Open EphraimHaber opened 3 months ago

EphraimHaber commented 3 months ago

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 commented 3 months ago

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.