tox-dev / filelock

A platform-independent file lock for Python.
https://py-filelock.readthedocs.io
The Unlicense
760 stars 107 forks source link

Use a metaclass to implement the singleton pattern #340

Closed kwist-sgr closed 4 months ago

kwist-sgr commented 4 months ago

Reason

More correct implementation of the singleton pattern.

There is a problem with the current implementation: class's __init__ to be called again whenever lock_class(is_singleton=True) is called, which is at least not obvious (also, it'll lead to the re-initializing instance's internal objects, e.g.: virtualenv/util/lock.py)

What was done in this PR

Related PRs

gaborbernat commented 4 months ago

@kwist-sgr https://github.com/tox-dev/sphinx-autodoc-typehints/actions/runs/9583644379/job/26425402570 seems broke the world had to yank it...

tamird commented 4 months ago

To be fair it seems virtualenv is violating the contract by not implementing the proper signature of __init__ here

gaborbernat commented 4 months ago

The thing is that all other args past the lockfile should be optional... or they were, so without making it a breaking change we can't suddenly require downstream to have this optional args...

ethanbb commented 4 months ago

~OK now the issue is using the default __new__ for BaseFileLock. Since __init__ doesn't accept kwargs, __new__ won't either. The way it previously worked was, if you wanted to add more arguments, you have to override __init__ but not __new__ since __new__ explicitly allowed unused keyword arguments (maybe a weird pattern) but now that function is gone from BaseFileLock, so it's using the same signature as __init__ by default.~

Hmm actually never mind, that doesn't make sense with the error

ethanbb commented 4 months ago

OK I understand @gaborbernat's explanation. Yeah looking further, beyond just the super call, the __call__ signature assumes that subclasses will all accept the same arguments (at minimum). And if that's not true, testing whether the singleton matches the passed-in parameters isn't possible at this point in the code in this way.

kwist-sgr commented 4 months ago

@gaborbernat Oh. I apologize. I'm going to create a fix