From 0ac78143f26d86072b36659ad27706bbb9e54013 Mon Sep 17 00:00:00 2001 From: Dima Gerasimov Date: Sun, 10 May 2020 21:32:48 +0100 Subject: [PATCH 1/3] add my.demo for testing out various approaches to configuring --- doc/CONFIGURING.org | 24 ++++++++++++++++--- my/demo.py | 56 +++++++++++++++++++++++++++++++++++++++++++++ tests/demo.py | 29 +++++++++++++++++++++++ tox.ini | 2 +- 4 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 my/demo.py create mode 100644 tests/demo.py diff --git a/doc/CONFIGURING.org b/doc/CONFIGURING.org index 6900f40..6006a68 100644 --- a/doc/CONFIGURING.org +++ b/doc/CONFIGURING.org @@ -187,6 +187,8 @@ I see mypy annotations as the only sane way to support it, because we also get ( - it doesn't support optional attributes (optional as in non-required, not as ~typing.Optional~), so it goes against (3) - prior to python 3.8, it's a part of =typing_extensions= rather than standard =typing=, so using it requires guarding the code with =if typing.TYPE_CHECKING=, which is a bit confusing and bloating. + TODO: check out [[https://mypy.readthedocs.io/en/stable/protocols.html#using-isinstance-with-protocols][@runtime_checkable]]? + - =NamedTuple= [[https://github.com/karlicoss/HPI/pull/45/commits/c877104b90c9d168eaec96e0e770e59048ce4465][Here]] I experimented with using ~NamedTuple~. @@ -232,7 +234,7 @@ class bluemaestro(user_config): } return cls(**params) -config = reddit.make_config() +config = bluemaestro.make_config() #+end_src I claim this solves pretty much everything: @@ -240,11 +242,27 @@ I claim this solves pretty much everything: - *(2)*: collaterally, we also solved it, because we can adapt for renames and other legacy config adaptations in ~make_config~ - *(3)*: supports default attributes, at no extra cost - *(4)*: the user config's attributes are available through the base class -- *(5)*: everything is transparent to mypy. However, it still lacks runtime checks. +- *(5)*: everything is mostly transparent to mypy. There are no runtime type checks yet, but I think possible to integrate with ~@dataclass~ - *(6)*: the dataclass header is easily readable, and it's possible to generate the docs automatically Downsides: -- the =make_config= bit is a little scary and manual, however, it can be extracted in a generic helper method +- inheriting from ~user_config~ means early import of =my.config= + + Generally it's better to keep everything as lazy as possible and defer loading to the first time the config is used. + This might be annoying at times, e.g. if you have a top-level import of you module, but no config. + + But considering that in 99% of cases config is going to be on the disk + and it's possible to do something dynamic like =del sys.modules['my.bluemastro']= to reload the config, I think it's a minor issue. + # TODO demonstrate in a test? + +- =make_config= allows for some mypy false negatives in the user config + + E.g. if you forgot =export_path= attribute, mypy would miss it. But you'd have a runtime failure, and the downstream code using config is still correctly type checked. + + Perhaps it will be better when [[https://github.com/python/mypy/issues/5374][this]] is fixed. +- the =make_config= bit is a little scary and manual + + However, it's extracted in a generic helper, and [[https://github.com/karlicoss/HPI/blob/d6f071e3b12ba1cd5a86ad80e3821bec004e6a6d/my/twitter/archive.py#L17][ends up pretty simple]] My conclusion is that I'm going with this approach for now. Note that at no stage in required any changes to the user configs, so if I missed something, it would be reversible. diff --git a/my/demo.py b/my/demo.py new file mode 100644 index 0000000..ae57b67 --- /dev/null +++ b/my/demo.py @@ -0,0 +1,56 @@ +''' +Just a demo module for testing and documentation purposes +''' + +from .core.common import Paths + +from datetime import tzinfo +import pytz + +from my.config import demo as user_config +from dataclasses import dataclass + +@dataclass +class demo(user_config): + data_path: Paths + username: str + timezone: tzinfo = pytz.utc + + +def config() -> demo: + from .core.cfg import make_config + config = make_config(demo) + return config + + + +from pathlib import Path +from typing import Sequence, Iterable +from datetime import datetime +from .core.common import Json, get_files + +@dataclass +class Item: + ''' + Some completely arbirary artificial stuff, just for testing + ''' + username: str + raw: Json + dt: datetime + + +def inputs() -> Sequence[Path]: + return get_files(config().data_path) + + +import json +def items() -> Iterable[Item]: + for f in inputs(): + dt = datetime.fromtimestamp(f.stat().st_mtime, tz=config().timezone) + j = json.loads(f.read_text()) + for raw in j: + yield Item( + username=config().username, + raw=raw, + dt=dt, + ) diff --git a/tests/demo.py b/tests/demo.py new file mode 100644 index 0000000..7298613 --- /dev/null +++ b/tests/demo.py @@ -0,0 +1,29 @@ +from pathlib import Path +from more_itertools import ilen + +# TODO NOTE: this wouldn't work because of an early my.config.demo import +# from my.demo import items + +def test_dynamic_config(tmp_path: Path) -> None: + import my.config + + class user_config: + username = 'user' + data_path = f'{tmp_path}/*.json' + my.config.demo = user_config # type: ignore[misc, assignment] + + from my.demo import items + [item1, item2] = items() + assert item1.username == 'user' + + +import pytest # type: ignore +@pytest.fixture(autouse=True) +def prepare(tmp_path: Path): + (tmp_path / 'data.json').write_text(''' +[ + {"key1": 1}, + {"key2": 2} +] +''') + yield diff --git a/tox.ini b/tox.ini index d11c30d..82efab2 100644 --- a/tox.ini +++ b/tox.ini @@ -13,7 +13,7 @@ commands = # todo these are probably not necessary anymore? python3 -c 'from my.config import stub as config; print(config.key)' python3 -c 'import my.config; import my.config.repos' # shouldn't fail at least - python3 -m pytest tests/misc.py tests/get_files.py tests/config.py::test_set_repo tests/config.py::test_environment_variable + python3 -m pytest tests/misc.py tests/get_files.py tests/config.py::test_set_repo tests/config.py::test_environment_variable tests/demo.py # TODO add; once I figure out porg depdencency?? tests/config.py # TODO run demo.py? just make sure with_my is a bit cleverer? # TODO e.g. under CI, rely on installing From 1d0ef82d32fa48fd1b59cc5bc24170331c4df79d Mon Sep 17 00:00:00 2001 From: Dima Gerasimov Date: Sun, 10 May 2020 21:57:55 +0100 Subject: [PATCH 2/3] Add test demonstrating unloading the module during dynamic configuration --- doc/CONFIGURING.org | 25 ++++++++++++++++++++----- tests/config.py | 2 ++ tests/demo.py | 42 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 6 deletions(-) diff --git a/doc/CONFIGURING.org b/doc/CONFIGURING.org index 6006a68..6c7c70e 100644 --- a/doc/CONFIGURING.org +++ b/doc/CONFIGURING.org @@ -20,11 +20,11 @@ Now, the requirements as I see it: 1. configuration should be *extremely* flexible - We need to make sure it's very easy to combine/filter/extend data without having to modify and rewrite the module code. + We need to make sure it's very easy to combine/filter/extend data without having to turn the module code inside out. This means using a powerful language for config, and realistically, a Turing complete. General: that means that you should be able to use powerful syntax, potentially running arbitrary code if - this is something you need (for whatever mad reason). It should be possible to override config attributes in runtime, if necessary. + this is something you need (for whatever mad reason). It should be possible to override config attributes *in runtime*, if necessary, without rewriting files on the filesystem. Specific: we've got Python already, so it makes a lot of sense to use it! @@ -160,6 +160,16 @@ from my.config import bluemaestro as user_config Let's go through requirements: - (1): *yes*, simply importing Python code is the most flexible you can get + In addition, in runtime, you can simply assign a new config if you need some dynamic hacking: + + #+begin_src python + class new_config: + export_path = '/some/hacky/dynamic/path' + my.config = new_config + #+end_src + + After that, =my.bluemaestro= would run against your new config. + - (2): *no*, but backwards compatibility is not necessary in the first version of the module - (3): *mostly*, although optional fields require extra work - (4): *yes*, whatever is in the config can immediately be used by the code @@ -176,7 +186,7 @@ I see mypy annotations as the only sane way to support it, because we also get ( However, it's using plain files and doesn't satisfy (1). Also not sure about (5). =file-config= allows using mypy annotations, but I'm not convinced they would be correctly typed with mypy, I think you need a plugin for that. - + - [[https://mypy.readthedocs.io/en/stable/protocols.html#simple-user-defined-protocols][Protocol]] I experimented with ~Protocol~ [[https://github.com/karlicoss/HPI/pull/45/commits/90b9d1d9c15abe3944913add5eaa5785cc3bffbc][here]]. @@ -205,7 +215,7 @@ I see mypy annotations as the only sane way to support it, because we also get ( Downsides: - we partially lost (5), because dynamic attributes are not transparent to mypy. - + My conclusion was using a *combined approach*: @@ -214,7 +224,7 @@ My conclusion was using a *combined approach*: Inheritance is a standard mechanism, which doesn't require any extra frameworks and plays well with other Python concepts. As a specific example: -#+begin_src python +,#+begin_src python from my.config import bluemaestro as user_config @dataclass @@ -264,6 +274,11 @@ Downsides: However, it's extracted in a generic helper, and [[https://github.com/karlicoss/HPI/blob/d6f071e3b12ba1cd5a86ad80e3821bec004e6a6d/my/twitter/archive.py#L17][ends up pretty simple]] +- inheriting from ~user_config~ requires it to be a =class= rather than an =object= + + A practical downside is you can't use something like ~SimpleNamespace~. + But considering you can define an ad-hoc =class= anywhere, this is fine? + My conclusion is that I'm going with this approach for now. Note that at no stage in required any changes to the user configs, so if I missed something, it would be reversible. diff --git a/tests/config.py b/tests/config.py index 508c387..523797a 100644 --- a/tests/config.py +++ b/tests/config.py @@ -1,6 +1,8 @@ from pathlib import Path +# TODO switch these from using SimpleNamespace + def setup_notes_path(notes: Path) -> None: # TODO reuse doc from my.cfg? from my.cfg import config diff --git a/tests/demo.py b/tests/demo.py index 7298613..93b777c 100644 --- a/tests/demo.py +++ b/tests/demo.py @@ -1,10 +1,11 @@ +import sys from pathlib import Path from more_itertools import ilen # TODO NOTE: this wouldn't work because of an early my.config.demo import # from my.demo import items -def test_dynamic_config(tmp_path: Path) -> None: +def test_dynamic_config_1(tmp_path: Path) -> None: import my.config class user_config: @@ -17,7 +18,46 @@ def test_dynamic_config(tmp_path: Path) -> None: assert item1.username == 'user' +# exactly the same test, but using a different config, to test out the behavious w.r.t. import order +def test_dynamic_config_2(tmp_path: Path) -> None: + # doesn't work without it! + # because the config from test_dybamic_config_1 is cached in my.demo.demo + del sys.modules['my.demo'] + + import my.config + + class user_config: + username = 'user2' + data_path = f'{tmp_path}/*.json' + my.config.demo = user_config # type: ignore[misc, assignment] + + from my.demo import items + [item1, item2] = items() + assert item1.username == 'user2' + + import pytest # type: ignore + +@pytest.mark.skip(reason="won't work at the moment because of inheritance") +def test_dynamic_config_simplenamespace(tmp_path: Path) -> None: + # doesn't work without it! + # because the config from test_dybamic_config_1 is cached in my.demo.demo + del sys.modules['my.demo'] + + import my.config + from types import SimpleNamespace + + user_config = SimpleNamespace( + username='user3', + data_path=f'{tmp_path}/*.json', + ) + my.config.demo = user_config # type: ignore[misc, assignment] + + from my.demo import config + assert config().username == 'user3' + + + @pytest.fixture(autouse=True) def prepare(tmp_path: Path): (tmp_path / 'data.json').write_text(''' From d7abff03fcddf311ccc6b70f116a80c9db6e8ac1 Mon Sep 17 00:00:00 2001 From: Dima Gerasimov Date: Sun, 10 May 2020 22:43:02 +0100 Subject: [PATCH 3/3] add dataclasses dependency for python<3.7 --- setup.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 701564a..b2c78e3 100644 --- a/setup.py +++ b/setup.py @@ -60,7 +60,11 @@ def main(): # TODO document these? 'logzero', 'cachew', - ] + ], + ':python_version<"3.7"': [ + # used for some modules... hopefully reasonable to have it as a default + 'dataclasses', + ], }, )