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

Switch Config to singleton design pattern #198

Merged
merged 8 commits into from
May 29, 2024
Merged

Switch Config to singleton design pattern #198

merged 8 commits into from
May 29, 2024

Conversation

olivierdelree
Copy link
Contributor

Closes #197.

We used to have a Config class with a regular __init__ constructor that would result in a new Config object being instantiated for each module that called it.
This PR restructures the class to be static and use the __new__ constructor. The class now keeps track of a single instance that is created with the first __new__ call and is returned with subsequent calls to the constructor. This has the advantage of synchronising the config across the whole application as only one config ever exists at the same time. Additionally, it means that the current syntax does not need to be modified throughout the application as Config() naturally makes use of __new__ alone if no __init__ exists.
This implementation assumes that only one config will ever be necessary for the application. However, if for some reason in the future (e.g., for simultaneous tests), we need multiple configs to exist at once, the class could be made to instead accept a config ID and keep track of a hashmap of configs.

This revamps the implementation of the `Config` class to a singleton
approach. Instead of offering a regular constructor to the rest of the
application, this keeps track of a single static instance of the config
and returns it on subsequent calls.
@olivierdelree olivierdelree self-assigned this May 27, 2024
@olivierdelree olivierdelree added enhancement New feature or request refactor Change how things are called or structured labels May 27, 2024
This allows saving to the same file we loaded from when saving after
initialisation and makes it possible to back up the config to do some
tests and restore it afterwards.
@olivierdelree
Copy link
Contributor Author

Regarding potentially using a custom config, should we keep track of the config path? Currently, if we load a config from a file, all's well. But if we modify the config and want to save it back to file after changing it, we don't know where the config came from, or even if it came from a custom path rather than the default.
This shouldn't be too much of a problem in most use cases as the users of the app will most likely always start the app from the same place and the config won't change but for debugging and testing, it would be nice to have ways to save and reload the config.
I'll update this PR with some additions for this but I am more than happy sticking to the first commit only if the additions don't appear worth it down the line.

Current methods were modifying the class attributes rather than
modifying the actual singleton (`_instance`), meaning modifications to
the instance were not actually reflected when saving, etc...
With this, the modifications actually happen on the instance and the
functions are also applied to that instance.
Since we now keep track of the config path in the config itself, we
should update it when saving it to a new location.
@olivierdelree
Copy link
Contributor Author

Looking at the currently opened issues, it seems this also somewhat addresses #116.
From this PR, it would become possible to open a custom config and share that with the whole application but it still requires a custom Python script to stand between the user and the application to specify a custom path. This is fine for tests but not necessarily too nice from an end-user point of view. If the changes proposed here seem acceptable, I can open a new PR that uses a basic CLI parser to allow passing a config path from the CLI when starting the app.
Let me know if you think that would be something of interest.

@olivierdelree olivierdelree marked this pull request as draft May 28, 2024 16:06
@olivierdelree olivierdelree marked this pull request as ready for review May 29, 2024 12:33
Copy link
Member

@celefthe celefthe left a comment

Choose a reason for hiding this comment

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

Very elegant!!

Check the comment re classes importing from object explicitly, otherwise happy to merge

@@ -2,18 +2,17 @@

# This file is part of visiomode.
# Copyright (c) 2020 Constantinos Eleftheriou <[email protected]>
# Copyright (c) 2024 Olivier Delree <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Yes nice please do remember to add this as you go, and we should also probably create an authors or contributors txt

src/visiomode/config.py Outdated Show resolved Hide resolved
@celefthe
Copy link
Member

Looking at the currently opened issues, it seems this also somewhat addresses #116. From this PR, it would become possible to open a custom config and share that with the whole application but it still requires a custom Python script to stand between the user and the application to specify a custom path. This is fine for tests but not necessarily too nice from an end-user point of view. If the changes proposed here seem acceptable, I can open a new PR that uses a basic CLI parser to allow passing a config path from the CLI when starting the app. Let me know if you think that would be something of interest.

Yeah I don't see why not, sounds like a useful addition and would also allow for easier troubleshooting on-the-fly on an individual device in the event the application doesn't start on account of a misconfiguration / the default settings not playing well on a particular device.

@celefthe
Copy link
Member

This implementation assumes that only one config will ever be necessary for the application. However, if for some reason in the future (e.g., for simultaneous tests), we need multiple configs to exist at once, the class could be made to instead accept a config ID and keep track of a hashmap of configs.

I think we can safely assume only one config will ever be necessary. I think baking-in the alternative introduces unnecessary complexity for what is a mild gain for testing imho

No reason to inherit from `object` since we're working with Python 3.
@olivierdelree
Copy link
Contributor Author

Happy to merge if you are!

@celefthe celefthe merged commit f700a12 into main May 29, 2024
2 checks passed
@celefthe celefthe deleted the od/issue197 branch May 29, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Change how things are called or structured
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration is not synchronised throughout the application
2 participants