-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add IDCClient() singleton feature #99
Conversation
@LennyN95 do you recommend any tutorials or books to learn more about singletons? |
@LennyN95 I don't object in principle, but can you give the rationale for adding this feature? I don't think I ever had a need to create more than one client. Or maybe that's exactly the point - to prevent user from creating multiple clients, since this should not ever be needed, but indeed can easily be done before this change? I assume you will resolve the conflicts. |
This here gives an excellent overview! Note, I didn't implement an actual singleton but rather a singleton factory method, which I believe is the most suited for our use case. It simplifies access while not impacting flexibility and testability. |
Sure @fedorov, I've multiple reasons in mind:
|
@LennyN95 I resolved the conflicts. |
@LennyN95 I hope you can look into CI errors! |
idc_index/__init__.py
Outdated
@@ -9,3 +9,5 @@ | |||
from ._version import version as __version__ | |||
|
|||
__all__ = ["__version__"] | |||
|
|||
from .index import IDCClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fedorov I added this import such that we can import IDCClient
directly, instead of importing index
and then accessing it via index.IDCClient
. I see this got removed, is this because we have any rule that prevents adding such imports on the package level or did it just got lost in the process? I prefer this way as it simplifies importing a lot and enhances ux in my opinion but I'm open for counter-arguments ;)
Some of the errors are related to the now missing import, so I wait for your reply before fixing them.
Thx for looking into this!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter doesn't like the unused import... I push a workaround, let me know what you think ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I just accepted the corrections that we proposed by the linter/ruff. Looks good to me!
No need for a global
client = IDCClient()
call, insteadIDCClient.client()
can be used directly. Upon first call a new IDCClient instance will be initialized and reused for all following calls.