From bc15e509cd94be364a0e61702fa000c2013a6032 Mon Sep 17 00:00:00 2001 From: lemon24 Date: Wed, 19 Aug 2020 16:59:17 +0300 Subject: [PATCH] Add config object helper. For #177. --- src/reader/_app/__init__.py | 16 ++--- src/reader/_app/cli.py | 2 +- src/reader/_app/wsgi.py | 9 ++- src/reader/_cli.py | 36 +++------- src/reader/_config.py | 102 +++++++++++++++++++++++----- tests/test_app.py | 5 +- tests/test_cli.py | 129 +++++++++++++++++++----------------- tests/test_config.py | 75 +++++++++++++++++++++ 8 files changed, 253 insertions(+), 121 deletions(-) create mode 100644 tests/test_config.py diff --git a/src/reader/_app/__init__.py b/src/reader/_app/__init__.py index b864e583..e75b42e9 100644 --- a/src/reader/_app/__init__.py +++ b/src/reader/_app/__init__.py @@ -30,7 +30,6 @@ from reader import InvalidSearchQueryError from reader import ParseError from reader import ReaderError -from reader._config import make_reader_from_config from reader._plugins import Loader from reader._plugins import LoaderError @@ -47,9 +46,8 @@ def get_reader(): if not hasattr(g, 'reader'): - g.reader = make_reader_from_config( - plugin_loader_cls=FlaskPluginLoader, - **current_app.config['READER_CONFIG']['reader'], + g.reader = current_app.config['READER_CONFIG'].make_reader( + 'default', plugin_loader_cls=FlaskPluginLoader ) return g.reader @@ -234,11 +232,9 @@ def preview(): # TODO: maybe cache stuff - # TODO: config should have a helper to do this - kwargs = current_app.config['READER_CONFIG']['reader'].copy() - kwargs['url'] = ':memory:' - current_app.config['READER_CONFIG']['reader'] - reader = make_reader_from_config(**kwargs, plugin_loader_cls=FlaskPluginLoader) + reader = current_app.config['READER_CONFIG'].make_reader( + 'default', url=':memory:', plugin_loader_cls=FlaskPluginLoader + ) reader.add_feed(url) @@ -463,6 +459,6 @@ def create_app(config): # app_context() needed for logging to work. with app.app_context(): - FlaskPluginLoader(config.get('app', {}).get('plugins', {})).load_plugins(app) + FlaskPluginLoader(config.merged('app').get('plugins', {})).load_plugins(app) return app diff --git a/src/reader/_app/cli.py b/src/reader/_app/cli.py index 8778b091..057f2d50 100644 --- a/src/reader/_app/cli.py +++ b/src/reader/_app/cli.py @@ -25,7 +25,7 @@ def serve(config, host, port, plugin, verbose): config['app']['plugins'] = dict.fromkeys(plugin) # FIXME: remove this once we make debug_storage a storage_arg - config['reader'].pop('debug_storage', None) + config['default']['reader'].pop('debug_storage', None) app = create_app(config) run_simple(host, port, app) diff --git a/src/reader/_app/wsgi.py b/src/reader/_app/wsgi.py index 27ce1e94..11506391 100644 --- a/src/reader/_app/wsgi.py +++ b/src/reader/_app/wsgi.py @@ -20,17 +20,16 @@ with open(os.environ[reader._CONFIG_ENVVAR]) as file: config = reader._config.load_config(file) else: - config = reader._config.load_config({'reader': {}, 'app': {}}) + config = reader._config.load_config({}) -# TODO: if we ever merge sections, this needs to become: config[*]['reader']['url'] = ... if reader._DB_ENVVAR in os.environ: - config['reader']['url'] = os.environ[reader._DB_ENVVAR] + config.all['reader']['url'] = os.environ[reader._DB_ENVVAR] if reader._PLUGIN_ENVVAR in os.environ: - config['reader']['plugins'] = dict.fromkeys( + config.all['reader']['plugins'] = dict.fromkeys( os.environ[reader._PLUGIN_ENVVAR].split() ) if reader._APP_PLUGIN_ENVVAR in os.environ: - config['app']['plugins'] = dict.fromkeys( + config.data['app']['plugins'] = dict.fromkeys( os.environ[reader._APP_PLUGIN_ENVVAR].split() ) diff --git a/src/reader/_cli.py b/src/reader/_cli.py index a44bf48e..e266e02a 100644 --- a/src/reader/_cli.py +++ b/src/reader/_cli.py @@ -12,7 +12,6 @@ from ._config import make_reader_from_config from ._plugins import LoaderError from ._sqlite_utils import DebugConnection -from .types import MISSING APP_NAME = reader.__name__ @@ -124,18 +123,6 @@ def wrapper(*args, **kwargs): return wrapper -def get_dict_path(d, path): - for key in path[:-1]: - d = d.get(key, {}) - return d.get(path[-1], MISSING) - - -def set_dict_path(d, path, value): - for key in path[:-1]: - d = d.setdefault(key, {}) - d[path[-1]] = value - - def config_option(*args, **kwargs): def callback(ctx, param, value): # TODO: the default file is allowed to not exist, a user specified file must exist @@ -145,9 +132,9 @@ def callback(ctx, param, value): except FileNotFoundError as e: if value != param.default: raise click.BadParameter(str(e), ctx=ctx, param=param) - config = {} + config = load_config({}) - ctx.default_map = config.get('cli', {}).get('defaults', {}) + ctx.default_map = config['cli'].get('defaults', {}) ctx.obj = config return config @@ -169,7 +156,8 @@ def pass_reader(fn): @functools.wraps(fn) def wrapper(*args, **kwargs): ctx = click.get_current_context().find_root() - reader = make_reader_with_plugins(**ctx.obj['reader']) + # TODO: replace with ctx.obj.make_reader('cli') when we get rid of debug_storage + reader = make_reader_with_plugins(**ctx.obj.merged('cli').get('reader', {})) ctx.call_on_close(reader.close) return fn(reader, *args, **kwargs) @@ -209,27 +197,23 @@ def cli(config, db, plugin, debug_storage): # (same for wsgi envvars) # NOTE: we can never use click defaults for --db/--plugin, because they would override the config always - # TODO: remove me after we have object - for key in 'reader', 'cli', 'app': - config.setdefault(key, {}) - - # TODO: if we ever merge sections, this needs to become: config[*]['reader']['url'] = ... if db: - config['reader']['url'] = db + config.all['reader']['url'] = db else: - if not config['reader'].get('url'): + # ... could be the 'cli' section, maybe... + if not config['default'].get('reader', {}).get('url'): try: db = get_default_db_path(create_dir=True) except Exception as e: abort("{}", e) - config['reader']['url'] = db + config.all['reader']['url'] = db if plugin: - config['reader']['plugins'] = dict.fromkeys(plugin) + config.all['reader']['plugins'] = dict.fromkeys(plugin) # until we make debug_storage a proper make_reader argument, # and we get rid of make_reader_with_plugins - config['reader']['debug_storage'] = debug_storage + config['default']['reader']['debug_storage'] = debug_storage @cli.command() diff --git a/src/reader/_config.py b/src/reader/_config.py index 450bf91e..a99a3058 100644 --- a/src/reader/_config.py +++ b/src/reader/_config.py @@ -5,14 +5,15 @@ """ from collections.abc import Mapping +from dataclasses import dataclass +from dataclasses import field from reader import make_reader from reader._plugins import import_string from reader._plugins import Loader -IMPORT_KWARGS = ('storage_cls', 'search_cls') -MERGE_KWARGS = ('plugins',) +MAKE_READER_IMPORT_KWARGS = ('storage_cls', 'search_cls') def make_reader_from_config(*, plugins=None, plugin_loader_cls=Loader, **kwargs): @@ -24,7 +25,7 @@ def make_reader_from_config(*, plugins=None, plugin_loader_cls=Loader, **kwargs) """ plugins = plugins or {} - for name in IMPORT_KWARGS: + for name in MAKE_READER_IMPORT_KWARGS: thing = kwargs.get(name) if thing and isinstance(thing, str): kwargs[name] = import_string(thing) @@ -40,7 +41,22 @@ def make_reader_from_config(*, plugins=None, plugin_loader_cls=Loader, **kwargs) return reader -def merge_config(*configs, merge_kwargs=MERGE_KWARGS): +def load_config(thing): + if isinstance(thing, Mapping): + config = thing + else: + import yaml + + config = yaml.safe_load(thing) + if not isinstance(config, Mapping): + raise ValueError("config must be a mapping") + + # TODO: validate / raise nicer exceptions here + + return Config(config, sections={'cli', 'app'}, merge_keys={'reader', 'plugins'}) + + +def _merge_config(*configs, merge_keys=()): """Merge multiple make_app_from_config() kwargs dicts into a single one. plugins is assumed to be a dict and is merged. All other keys are replaced. @@ -51,26 +67,80 @@ def merge_config(*configs, merge_kwargs=MERGE_KWARGS): for config in configs: config = config.copy() - for name in MERGE_KWARGS: + for name in merge_keys: if name in config: to_merge.setdefault(name, []).append(config.pop(name)) rv.update(config) for name, dicts in to_merge.items(): - rv[name] = merge_config(*dicts, merge_kwargs=()) + rv[name] = _merge_config(*dicts, merge_keys=()) return rv -def load_config(thing): - if isinstance(thing, Mapping): - config = thing - else: - import yaml +@dataclass +class Config: - config = yaml.safe_load(thing) - if not isinstance(config, Mapping): - raise ValueError("config must be a mapping") + data: dict = field(default_factory=dict) + sections: set = field(default_factory=set) + merge_keys: set = field(default_factory=set) + default_section: str = 'default' - # TODO: validate / raise nicer exceptions here - return config + def __post_init__(self): + self.sections.add(self.default_section) + + unknown_sections = self.data.keys() - self.sections + + if self.default_section in self.data: + if unknown_sections: + raise ValueError(f"unknown sections in config: {unknown_sections!r}") + else: + self.data[self.default_section] = { + section: self.data.pop(section) for section in unknown_sections + } + + for section in self.sections: + self.data.setdefault(section, {}) + + def merged(self, section, overrides=None): + if section not in self.sections: + raise ValueError(f"unknown section: {section!r}") + + return _merge_config( + self.data[self.default_section], + self.data[section], + overrides or {}, + merge_keys=self.merge_keys, + ) + + def make_reader(self, section, **kwargs): + return make_reader_from_config( + **self.merged(section, {'reader': kwargs}).get('reader', {}), + ) + + @property + def all(self): + return MultiMapping(list(self.data.values())) + + def __getitem__(self, key): + return self.data[key] + + +@dataclass +class MultiMapping: + + mappings: list = field(default_factory=list) + default_factory: callable = dict + + def __getitem__(self, key): + return MultiMapping( + [ + mapping.setdefault(key, self.default_factory()) + for mapping in self.mappings + ], + self.default_factory, + ) + + def __setitem__(self, key, value): + for mapping in self.mappings: + mapping[key] = value diff --git a/tests/test_app.py b/tests/test_app.py index e6274dd7..a2d94787 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -8,12 +8,13 @@ from reader import make_reader from reader._app import create_app +from reader._config import load_config from reader._config import make_reader_from_config @pytest.fixture def browser(db_path): - app = create_app({'reader': {'url': db_path}}) + app = create_app(load_config({'reader': {'url': db_path}})) session = requests.Session() session.mount('http://app/', wsgiadapter.WSGIAdapter(app)) browser = mechanicalsoup.StatefulBrowser(session) @@ -106,7 +107,7 @@ def app_make_reader(**kwargs): return reader # this is brittle, it may break if we change how we use make_reader in app - monkeypatch.setattr('reader._app.make_reader_from_config', app_make_reader) + monkeypatch.setattr('reader._config.make_reader_from_config', app_make_reader) reader = app_make_reader(url=db_path) diff --git a/tests/test_cli.py b/tests/test_cli.py index 227e4c0c..d33aa16c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -8,9 +8,6 @@ from reader import Reader from reader._cli import cli from reader._cli import config_option -from reader._cli import get_dict_path -from reader._cli import set_dict_path -from reader._config import merge_config from reader.types import MISSING @@ -194,90 +191,100 @@ def create_app(config): runner.invoke(cli, ['--db', db_path, 'serve'], catch_exceptions=False) assert excinfo.value is exception - assert create_app.config == { + assert create_app.config.merged('app') == { + 'reader': {'url': db_path}, + } + assert create_app.config.merged('default') == { 'reader': {'url': db_path}, - 'app': {}, - 'cli': {}, } def test_config_option(tmpdir): final_config = None - @click.command() + @click.group() + @click.option('--db') + @click.option('--plugin', multiple=True) @config_option('--config') - @click.option('--aaa', default='aaa-cli-default') - @click.option('--bbb/--no-bbb', default=False) - @click.option('--ccc', type=int, default=1) - @click.option('--ddd', default='ddd-cli-default') - @click.option('--eee', type=int, default=1) @click.pass_obj - def command(config, aaa, bbb, ccc, ddd, eee, **kwargs): - options = { - 'aaa': aaa, - 'bbb': bbb, - 'ccc': ccc, - 'ddd': ddd, - 'eee': eee, - } - - config.setdefault('command', {}) - config['command'] = merge_config(config['command'], options) + def cli(config, db, plugin): + if db: + config.all['reader']['url'] = db + if plugin: + config.all['reader']['plugins'] = dict.fromkeys(plugin) + nonlocal final_config + final_config = config.merged('cli') + + @cli.command() + @click.pass_obj + def update(config): + pass + @cli.command() + @click.option('--plugin', multiple=True) + @click.pass_obj + def serve(config, plugin): + if plugin: + config.data['app']['plugins'] = dict.fromkeys(plugin) nonlocal final_config - final_config = config + final_config = config.merged('app') config_path = tmpdir.join('config.yaml') config_path.write( yaml.safe_dump( { - 'command': { - 'ccc': 3, - 'eee': 3, - 'ddd': None, - 'fff': 'fff-config-value', + 'reader': { + 'url': 'config-reader-url', + 'plugins': {'config-reader-plugins': {}}, }, 'cli': { - 'defaults': {'bbb': True, 'ccc': 2, 'eee': 2,}, - 'other-cli-stuff': 'stuff', + 'reader': {'url': 'config-cli-url'}, + 'defaults': {'serve': {'plugin': ['defaults-app-plugins']}}, }, + 'app': {'plugins': {'config-app-plugins': {}},}, } ) ) runner = CliRunner() - result = runner.invoke( - command, - ['--config', str(config_path), '--ddd', 'ddd-user-value', '--no-bbb'], - catch_exceptions=False, - ) + invoke = lambda *args: runner.invoke(*args, catch_exceptions=False) - # config[command] < cli default < config[cli][defaults] < cli envvar,option - assert final_config['command'] == { - # cli default - 'aaa': 'aaa-cli-default', - # cli option - 'bbb': False, - # config[cli][defaults] - 'ccc': 2, - # cli option - 'ddd': 'ddd-user-value', - # config[cli][default], not config[command], because there was no override - 'eee': 2, - # config[command], because it is not a CLI option - 'fff': 'fff-config-value', + invoke(cli, ['--config', str(config_path), 'update']) + assert final_config['reader'] == { + 'url': 'config-cli-url', + 'plugins': {'config-reader-plugins': {}}, } + invoke(cli, ['--config', str(config_path), '--db', 'user-url', 'update']) + assert final_config['reader'] == { + 'url': 'user-url', + 'plugins': {'config-reader-plugins': {}}, + } -def test_get_dict_path(): - d = {'a': {'b': 'c'}} - assert get_dict_path(d, ('a', 'b')) == 'c' - assert get_dict_path(d, ('a', 'd', 'e')) is MISSING - + invoke(cli, ['--config', str(config_path), 'serve']) + assert final_config == { + 'reader': { + 'url': 'config-reader-url', + 'plugins': {'config-reader-plugins': {}}, + }, + 'plugins': {'defaults-app-plugins': None}, + } -def test_set_dict_path(): - d = {'a': {'b': 'c'}} - set_dict_path(d, ('a', 'b'), 'd') - assert d == {'a': {'b': 'd'}} - set_dict_path(d, ('a', 'e', 'f'), 'g') - assert d == {'a': {'b': 'd', 'e': {'f': 'g'}}} + invoke( + cli, + [ + '--config', + str(config_path), + '--db', + 'user-url', + '--plugin', + 'user-plugins', + 'serve', + '--plugin', + 'user-app-plugins', + ], + ) + assert final_config == { + 'reader': {'url': 'user-url', 'plugins': {'user-plugins': None},}, + 'plugins': {'user-app-plugins': None}, + } diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 00000000..bf7efcdb --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,75 @@ +import pytest + +from reader._config import Config + + +CONFIG_INIT_DATA = [ + (Config({}), {'default': {}}), + (Config({}, sections={'cli', 'app'}), {'default': {}, 'cli': {}, 'app': {}}), + ( + Config({'reader': {'k': 'v'}}, sections={'cli', 'app'}), + {'default': {'reader': {'k': 'v'}}, 'cli': {}, 'app': {}}, + ), + ( + Config({'default': {'reader': {'k': 'v'}}}, sections={'cli', 'app'}), + {'default': {'reader': {'k': 'v'}}, 'cli': {}, 'app': {}}, + ), +] + + +@pytest.mark.parametrize('config, data', CONFIG_INIT_DATA) +def test_config_init(config, data): + assert config.data == data + + +def test_config_init_error(): + with pytest.raises(ValueError): + Config({'default': {'reader': {}}, 'reader': {}}) + + +def test_config_merged(): + config = Config( + { + 'url': 'default-url', + 'plugins': {'default-plugin': None, 'another-plugin': 1}, + 'cli': {'url': 'cli-url'}, + 'app': {'plugins': {'app-plugin': None, 'another-plugin': 2}}, + }, + sections={'cli', 'app'}, + merge_keys={'plugins',}, + ) + + assert config.merged('cli') == { + 'url': 'cli-url', + 'plugins': {'default-plugin': None, 'another-plugin': 1}, + } + + assert config.merged('app') == { + 'url': 'default-url', + 'plugins': {'default-plugin': None, 'another-plugin': 2, 'app-plugin': None}, + } + + +def test_config_all(): + config = Config( + { + 'url': 'default-url', + 'nested': {'default-key': 'default-nested'}, + 'cli': {'url': 'cli-url', 'nested': {'cli-key': 'cli-nested'},}, + }, + sections={'cli', 'app'}, + merge_keys={'nested',}, + ) + + config.all['url'] = 'new-url' + assert config.data == { + 'default': {'url': 'new-url', 'nested': {'default-key': 'default-nested'},}, + 'cli': {'url': 'new-url', 'nested': {'cli-key': 'cli-nested'},}, + 'app': {'url': 'new-url',}, + } + + config.all['nested'] = {'new-key': 'new-value'} + assert config.data == dict.fromkeys( + ('default', 'cli', 'app'), + {'url': 'new-url', 'nested': {'new-key': 'new-value'},}, + )