Make pydantic based config system #141
Conversation
Functions that work with the new system but have backwards compatible signatures were made in the new config.py file; functions that weren't being used anywhere were not replicated. The old script.py can be deleted once all references to it has been safely replaced.
| @@ -0,0 +1 @@ | |||
| from . import webdav as webdav | |||
There was a problem hiding this comment.
It isn't needed syntactically, but running mypy in strict mode will complain about it. Although I can remove it because we haven't really decided on whether we are going to enforce a type-checker in the first place.
There was a problem hiding this comment.
Indeed. However, thinking about this, do we need the import at all? Even if the global variable (get_logger) within webdav.py would be needed, it could be initialized lazily upon import. (It should be removed anyway, though; as mentioned below, "caching" loggers is not needed, Python handles that for you.)
python/smashbox/config.py
Outdated
| from pydantic import BaseSettings | ||
|
|
||
|
|
||
| logger = None |
There was a problem hiding this comment.
Global variables are the root of evil. Generally, you should try to avoid them.
With Python's logging facilities, you do not need to "cache" logger objects anyway. Python handles this internally. Please see below for more information.
python/smashbox/config.py
Outdated
| if not logger: | ||
| if level is None: | ||
| level = logging.INFO # change here to DEBUG if you want to debug config stuff | ||
| logging.basicConfig(level=level) |
There was a problem hiding this comment.
You can set the logger's log level here if you want to, but the global call should be moved to the entry script.
I know this can be confusing. In Python, loggers can have levels. All messages lower than the logger's level will be suppressed. If the log message is not suppressed by the logger, the global configuration is then checked, if I remember this correctly. If the log message passes this check, it should be displayed.
python/smashbox/config.py
Outdated
|
|
||
| # this should probably be moved into a utilities module | ||
| def get_logger(name: str = "config", level: int | None = None) -> logging.Logger: | ||
| global logger |
There was a problem hiding this comment.
There is no need to cache the logger, Python does this internally. You can call getLogger as often as you want to, it will return the same object every time (at least when not using multithreading, of course). The method can be considered idempotent.
| def oc_server_datadirectory(self) -> str: | ||
| return os.path.join("/var/www/html", self.oc_root, "data") | ||
|
|
||
| # these methods exists for backwards compatibility |
There was a problem hiding this comment.
Do you intend to refactor those in the future? As demonstrated correctly below, Python's getattr and dict facilities are perfect Pythonic replacements.
| with open(fp, "r") as file: | ||
| return Configuration(**yaml.load(file)) | ||
|
|
||
| def configure_from_blob(fp: Path | str) -> Configuration: |
There was a problem hiding this comment.
Code style wise, you should always have at least two blank lines to separate methods.
You may want to include pep8/flake8 as development dependencies and run these on your code changes.
There was a problem hiding this comment.
I've added black, but forgot to run it this time 😅 I can set up pre-commit to automatically run these hooks every time you commit, but should I include that with this PR, or make a new one?
There was a problem hiding this comment.
I'd suggest to handle this in a follow-up PR. I'd also like to have a GitHub actions check that checks the formatting with black.
|
Looks quite good. I left some suggestions. |
|
Never heard of before. @SamuAlfageme what do you think about that new dependency? |
|
@anand2312 @fmoc It indeed looks nice. You can go ahead and include it in your working branch. You will have the chance to demonstrate how it improves the existing logging libraries. |
All logging calls have to be slowly converted after this.
|
@anand2312 is this ready for another round of review after your last commit? Would you be up to running a demonstration of your changes later this week? |
|
@SamuAlfageme there's a little more stuff left, I'll push them by tomorrow |
This reverts commit 21ec9ea.
|
I reverted the loguru commit as it would need a big change (changing all logger calls to use loguru), and it would be confusing if I did that work in this same PR. |
The configuration system has been revamped by making pydantic
do the parsing for us.
Functions that work with the new system but have backwards compatible
signatures were made in the new config.py file; functions that weren't
being used anywhere were not replicated.
The old script.py can be deleted once all references to it has been
safely replaced.