From 6aed6bca93ab910cc1ceb7adf540fd77d8410a75 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 25 Sep 2017 19:32:06 +0200 Subject: [PATCH] Make loading autoconfig.yml opt-in when a config.py exists This lets the user control the precedence those files should have, and also simplifies the code quite a bit. Fixes #2975 --- doc/help/configuring.asciidoc | 17 ++++---- qutebrowser/config/configexc.py | 6 +++ qutebrowser/config/configfiles.py | 34 ++++++++++----- qutebrowser/config/configinit.py | 41 +++++++----------- tests/unit/config/test_configexc.py | 7 ++++ tests/unit/config/test_configfiles.py | 60 +++++++++++---------------- tests/unit/config/test_configinit.py | 30 ++++++++------ 7 files changed, 103 insertions(+), 92 deletions(-) diff --git a/doc/help/configuring.asciidoc b/doc/help/configuring.asciidoc index b68e0bedc..afb6ec8e6 100644 --- a/doc/help/configuring.asciidoc +++ b/doc/help/configuring.asciidoc @@ -204,21 +204,22 @@ config.bind(',v', 'spawn mpv {url}') To suppress loading of any default keybindings, you can set `c.bindings.default = {}`. -Prevent loading `autoconfig.yml` -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Loading `autoconfig.yml` +~~~~~~~~~~~~~~~~~~~~~~~~ -If you want all customization done via `:set`, `:bind` and `:unbind` to be -temporary, you can suppress loading `autoconfig.yml` in your `config.py` by -doing: +By default, all customization done via `:set`, `:bind` and `:unbind` is +temporary as soon as a `config.py` exists. The settings done that way are always +saved in the `autoconfig.yml` file, but you'll need to explicitly load it in +your `config.py` by doing: .config.py: [source,python] ---- -config.load_autoconfig = False +config.load_autoconfig() ---- -Note that the settings are still saved in `autoconfig.yml` that way, but then -not loaded on start. +If you do so at the top of your file, your `config.py` settings will take +precedence as they overwrite the settings done in `autoconfig.yml`. Importing other modules ~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/qutebrowser/config/configexc.py b/qutebrowser/config/configexc.py index 4d283d21f..0d20bb09d 100644 --- a/qutebrowser/config/configexc.py +++ b/qutebrowser/config/configexc.py @@ -94,6 +94,12 @@ class ConfigErrorDesc: def __str__(self): return '{}: {}'.format(self.text, self.exception) + def with_text(self, text): + """Get a new ConfigErrorDesc with the given text appended.""" + return self.__class__(text='{} ({})'.format(self.text, text), + exception=self.exception, + traceback=self.traceback) + class ConfigFileErrors(Error): diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 62cd6088c..a017d025d 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -176,7 +176,6 @@ class ConfigAPI: Attributes: _config: The main Config object to use. _keyconfig: The KeyConfig object. - load_autoconfig: Whether autoconfig.yml should be loaded. errors: Errors which occurred while setting options. configdir: The qutebrowser config directory, as pathlib.Path. datadir: The qutebrowser data directory, as pathlib.Path. @@ -185,7 +184,6 @@ class ConfigAPI: def __init__(self, conf, keyconfig): self._config = conf self._keyconfig = keyconfig - self.load_autoconfig = True self.errors = [] self.configdir = pathlib.Path(standarddir.config()) self.datadir = pathlib.Path(standarddir.data()) @@ -194,6 +192,10 @@ class ConfigAPI: def _handle_error(self, action, name): try: yield + except configexc.ConfigFileErrors as e: + for err in e.errors: + new_err = err.with_text(e.basename) + self.errors.append(new_err) except configexc.Error as e: text = "While {} '{}'".format(action, name) self.errors.append(configexc.ConfigErrorDesc(text, e)) @@ -202,6 +204,10 @@ class ConfigAPI: """Do work which needs to be done after reading config.py.""" self._config.update_mutables() + def load_autoconfig(self): + with self._handle_error('reading', 'autoconfig.yml'): + read_autoconfig() + def get(self, name): with self._handle_error('getting', name): return self._config.get_obj(name) @@ -223,20 +229,15 @@ class ConfigAPI: self._keyconfig.unbind(key, mode=mode) -def read_config_py(filename=None, raising=False): +def read_config_py(filename, raising=False): """Read a config.py file. Arguments; + filename: The name of the file to read. raising: Raise exceptions happening in config.py. This is needed during tests to use pytest's inspection. """ api = ConfigAPI(config.instance, config.key_instance) - - if filename is None: - filename = os.path.join(standarddir.config(), 'config.py') - if not os.path.exists(filename): - return api - container = config.ConfigContainer(config.instance, configapi=api) basename = os.path.basename(filename) @@ -282,7 +283,20 @@ def read_config_py(filename=None, raising=False): exception=e, traceback=traceback.format_exc())) api.finalize() - return api + + if api.errors: + raise configexc.ConfigFileErrors('config.py', api.errors) + + +def read_autoconfig(): + """Read the autoconfig.yml file.""" + try: + config.instance.read_yaml() + except configexc.ConfigFileErrors as e: + raise # caught in outer block + except configexc.Error as e: + desc = configexc.ConfigErrorDesc("Error", e) + raise configexc.ConfigFileErrors('autoconfig.yml', [desc]) @contextlib.contextmanager diff --git a/qutebrowser/config/configinit.py b/qutebrowser/config/configinit.py index 27a37b5e9..a64438b83 100644 --- a/qutebrowser/config/configinit.py +++ b/qutebrowser/config/configinit.py @@ -19,18 +19,19 @@ """Initialization of the configuration.""" +import os.path import sys from PyQt5.QtWidgets import QMessageBox from qutebrowser.config import (config, configdata, configfiles, configtypes, configexc) -from qutebrowser.utils import objreg, qtutils, usertypes, log +from qutebrowser.utils import objreg, qtutils, usertypes, log, standarddir from qutebrowser.misc import earlyinit, msgbox, objects -# Errors which happened during init, so we can show a message box. -_init_errors = [] +# Error which happened during init, so we can show a message box. +_init_errors = None def early_init(args): @@ -52,29 +53,17 @@ def early_init(args): config.key_instance) objreg.register('config-commands', config_commands) - config_api = None + config_file = os.path.join(standarddir.config(), 'config.py') try: - config_api = configfiles.read_config_py() - # Raised here so we get the config_api back. - if config_api.errors: - raise configexc.ConfigFileErrors('config.py', config_api.errors) + if os.path.exists(config_file): + configfiles.read_config_py(config_file) + else: + configfiles.read_autoconfig() except configexc.ConfigFileErrors as e: - log.config.exception("Error while loading config.py") - _init_errors.append(e) - - try: - if getattr(config_api, 'load_autoconfig', True): - try: - config.instance.read_yaml() - except configexc.ConfigFileErrors as e: - raise # caught in outer block - except configexc.Error as e: - desc = configexc.ConfigErrorDesc("Error", e) - raise configexc.ConfigFileErrors('autoconfig.yml', [desc]) - except configexc.ConfigFileErrors as e: - log.config.exception("Error while loading config.py") - _init_errors.append(e) + log.config.exception("Error while loading {}".format(e.basename)) + global _init_errors + _init_errors = e configfiles.init() @@ -109,14 +98,14 @@ def get_backend(args): def late_init(save_manager): """Initialize the rest of the config after the QApplication is created.""" global _init_errors - for err in _init_errors: + if _init_errors is not None: errbox = msgbox.msgbox(parent=None, title="Error while reading config", - text=err.to_html(), + text=_init_errors.to_html(), icon=QMessageBox.Warning, plain_text=False) errbox.exec_() - _init_errors = [] + _init_errors = None config.instance.init_save_manager(save_manager) configfiles.state.init_save_manager(save_manager) diff --git a/tests/unit/config/test_configexc.py b/tests/unit/config/test_configexc.py index 8eaa21f05..024bbb1d0 100644 --- a/tests/unit/config/test_configexc.py +++ b/tests/unit/config/test_configexc.py @@ -49,6 +49,13 @@ def test_duplicate_key_error(): assert str(e) == "Duplicate key asdf" +def test_desc_with_text(): + """Test ConfigErrorDesc.with_text.""" + old = configexc.ConfigErrorDesc("Error text", Exception("Exception text")) + new = old.with_text("additional text") + assert str(new) == 'Error text (additional text): Exception text' + + @pytest.fixture def errors(): """Get a ConfigFileErrors object.""" diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index a2ed4099a..7ce29484a 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -222,9 +222,15 @@ class ConfPy: def read(self, error=False): """Read the config.py via configfiles and check for errors.""" - api = configfiles.read_config_py(self.filename, raising=not error) - assert len(api.errors) == (1 if error else 0) - return api + if error: + with pytest.raises(configexc.ConfigFileErrors) as excinfo: + configfiles.read_config_py(self.filename) + errors = excinfo.value.errors + assert len(errors) == 1 + return errors[0] + else: + configfiles.read_config_py(self.filename, raising=True) + return None def write_qbmodule(self): self.write('import qbmodule', @@ -263,8 +269,7 @@ class TestConfigPyModules: confpy.write_qbmodule() qbmodulepy.write('def run(config):', ' 1/0') - api = confpy.read(error=True) - error = api.errors[0] + error = confpy.read(error=True) assert error.text == "Unhandled exception" assert isinstance(error.exception, ZeroDivisionError) @@ -277,8 +282,7 @@ class TestConfigPyModules: confpy.write('import foobar', 'foobar.run(config)') - api = confpy.read(error=True) - error = api.errors[0] + error = confpy.read(error=True) assert error.text == "Unhandled exception" assert isinstance(error.exception, ImportError) @@ -367,8 +371,7 @@ class TestConfigPy: def test_bind_duplicate_key(self, confpy): """Make sure we get a nice error message on duplicate key bindings.""" confpy.write("config.bind('H', 'message-info back')") - api = confpy.read(error=True) - error = api.errors[0] + error = confpy.read(error=True) expected = "Duplicate key H - use force=True to override!" assert str(error.exception) == expected @@ -390,17 +393,6 @@ class TestConfigPy: assert config.instance._values['aliases']['foo'] == 'message-info foo' assert config.instance._values['aliases']['bar'] == 'message-info bar' - def test_reading_default_location(self, config_tmpdir, data_tmpdir): - (config_tmpdir / 'config.py').write_text( - 'c.colors.hints.bg = "red"', 'utf-8') - configfiles.read_config_py() - assert config.instance._values['colors.hints.bg'] == 'red' - - def test_reading_missing_default_location(self, config_tmpdir, - data_tmpdir): - assert not (config_tmpdir / 'config.py').exists() - configfiles.read_config_py() # Should not crash - def test_oserror(self, tmpdir, data_tmpdir, config_tmpdir): with pytest.raises(configexc.ConfigFileErrors) as excinfo: configfiles.read_config_py(str(tmpdir / 'foo')) @@ -443,12 +435,9 @@ class TestConfigPy: assert " ^" in tblines def test_unhandled_exception(self, confpy): - confpy.write("config.load_autoconfig = False", "1/0") + confpy.write("1/0") + error = confpy.read(error=True) - api = confpy.read(error=True) - error = api.errors[0] - - assert not api.load_autoconfig assert error.text == "Unhandled exception" assert isinstance(error.exception, ZeroDivisionError) @@ -460,8 +449,7 @@ class TestConfigPy: def test_config_val(self, confpy): """Using config.val should not work in config.py files.""" confpy.write("config.val.colors.hints.bg = 'red'") - api = confpy.read(error=True) - error = api.errors[0] + error = confpy.read(error=True) assert error.text == "Unhandled exception" assert isinstance(error.exception, AttributeError) @@ -470,11 +458,9 @@ class TestConfigPy: @pytest.mark.parametrize('line', ["c.foo = 42", "config.set('foo', 42)"]) def test_config_error(self, confpy, line): - confpy.write(line, "config.load_autoconfig = False") - api = confpy.read(error=True) - error = api.errors[0] + confpy.write(line) + error = confpy.read(error=True) - assert not api.load_autoconfig assert error.text == "While setting 'foo'" assert isinstance(error.exception, configexc.NoOptionError) assert str(error.exception) == "No option 'foo'" @@ -482,16 +468,20 @@ class TestConfigPy: def test_multiple_errors(self, confpy): confpy.write("c.foo = 42", "config.set('foo', 42)", "1/0") - api = configfiles.read_config_py(confpy.filename) - assert len(api.errors) == 3 - for error in api.errors[:2]: + with pytest.raises(configexc.ConfigFileErrors) as excinfo: + configfiles.read_config_py(confpy.filename) + + errors = excinfo.value.errors + assert len(errors) == 3 + + for error in errors[:2]: assert error.text == "While setting 'foo'" assert isinstance(error.exception, configexc.NoOptionError) assert str(error.exception) == "No option 'foo'" assert error.traceback is None - error = api.errors[2] + error = errors[2] assert error.text == "Unhandled exception" assert isinstance(error.exception, ZeroDivisionError) assert error.traceback is not None diff --git a/tests/unit/config/test_configinit.py b/tests/unit/config/test_configinit.py index fef352584..d13624c66 100644 --- a/tests/unit/config/test_configinit.py +++ b/tests/unit/config/test_configinit.py @@ -38,7 +38,7 @@ def init_patch(qapp, fake_save_manager, monkeypatch, config_tmpdir, monkeypatch.setattr(config, 'instance', None) monkeypatch.setattr(config, 'key_instance', None) monkeypatch.setattr(config, 'change_filters', []) - monkeypatch.setattr(configinit, '_init_errors', []) + monkeypatch.setattr(configinit, '_init_errors', None) # Make sure we get no SSL warning monkeypatch.setattr(configinit.earlyinit, 'check_backend_ssl_support', lambda _backend: None) @@ -73,8 +73,8 @@ def test_early_init(init_patch, config_tmpdir, caplog, fake_args, if config_py: config_py_lines = ['c.colors.hints.bg = "red"'] - if not load_autoconfig: - config_py_lines.append('config.load_autoconfig = False') + if load_autoconfig: + config_py_lines.append('config.load_autoconfig()') if config_py == 'error': config_py_lines.append('c.foo = 42') config_py_file.write_text('\n'.join(config_py_lines), @@ -85,21 +85,25 @@ def test_early_init(init_patch, config_tmpdir, caplog, fake_args, # Check error messages expected_errors = [] - if config_py == 'error': - expected_errors.append( - "Errors occurred while reading config.py:\n" - " While setting 'foo': No option 'foo'") + if load_autoconfig or not config_py: - error = "Errors occurred while reading autoconfig.yml:\n" + suffix = ' (autoconfig.yml)' if config_py else '' if invalid_yaml == '42': - error += " While loading data: Toplevel object is not a dict" + error = ("While loading data{}: Toplevel object is not a dict" + .format(suffix)) expected_errors.append(error) elif invalid_yaml == 'wrong-type': - error += (" Error: Invalid value 'True' - expected a value of " - "type str but got bool.") + error = ("Error{}: Invalid value 'True' - expected a value of " + "type str but got bool.".format(suffix)) expected_errors.append(error) + if config_py == 'error': + expected_errors.append("While setting 'foo': No option 'foo'") + + if configinit._init_errors is None: + actual_errors = [] + else: + actual_errors = [str(err) for err in configinit._init_errors.errors] - actual_errors = [str(err) for err in configinit._init_errors] assert actual_errors == expected_errors # Make sure things have been init'ed @@ -134,7 +138,7 @@ def test_late_init(init_patch, monkeypatch, fake_save_manager, fake_args, if errors: err = configexc.ConfigErrorDesc("Error text", Exception("Exception")) errs = configexc.ConfigFileErrors("config.py", [err]) - monkeypatch.setattr(configinit, '_init_errors', [errs]) + monkeypatch.setattr(configinit, '_init_errors', errs) msgbox_mock = mocker.patch('qutebrowser.config.configinit.msgbox.msgbox', autospec=True)