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

Use a metaclass to implement the singleton pattern #340

Merged
merged 8 commits into from
Jun 19, 2024

Conversation

kwist-sgr
Copy link
Contributor

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

  • added classes FileLockMeta/AsyncFileLockMeta used as metaclasses for BaseFileLock/BaseAsyncFileLock
  • added a test that checks that in the case of a singleton, the __init__ method is called only once

Related PRs

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failing.

@gaborbernat gaborbernat enabled auto-merge (squash) June 19, 2024 13:52
@gaborbernat gaborbernat merged commit 192f1ef into tox-dev:main Jun 19, 2024
30 checks passed
@gaborbernat
Copy link
Member

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

@tamird
Copy link

tamird commented Jun 19, 2024

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

@gaborbernat
Copy link
Member

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
Copy link
Contributor

ethanbb commented Jun 19, 2024

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
Copy link
Contributor

ethanbb commented Jun 19, 2024

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
Copy link
Contributor Author

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

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.

None yet

4 participants