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

@@ -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
+By default, all customization done via `:set`, `:bind` and `:unbind` is
-temporary, you can suppress loading `autoconfig.yml` in your `config.py` by
+temporary as soon as a `config.py` exists. The settings done that way are always
-doing:
+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
+If you do so at the top of your file, your `config.py` settings will take
-not loaded on start.
+precedence as they overwrite the settings done in `autoconfig.yml`.
 
 Importing other modules
 ~~~~~~~~~~~~~~~~~~~~~~~

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):
 

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

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.
+# Error which happened during init, so we can show a message box.
-_init_errors = []
+_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()
+        if os.path.exists(config_file):
-        # Raised here so we get the config_api back.
+            configfiles.read_config_py(config_file)
-        if config_api.errors:
+        else:
-            raise configexc.ConfigFileErrors('config.py', config_api.errors)
+            configfiles.read_autoconfig()
     except configexc.ConfigFileErrors as e:
-        log.config.exception("Error while loading config.py")
+        log.config.exception("Error while loading {}".format(e.basename))
-        _init_errors.append(e)
+        global _init_errors
-
+        _init_errors = 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)
 
     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)

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."""

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)
+        if error:
-        assert len(api.errors) == (1 if error else 0)
+            with pytest.raises(configexc.ConfigFileErrors) as excinfo:
-        return api
+                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 = confpy.read(error=True)
-        error = api.errors[0]
 
         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 = confpy.read(error=True)
-        error = api.errors[0]
 
         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 = confpy.read(error=True)
-        error = api.errors[0]
 
         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 = confpy.read(error=True)
-        error = api.errors[0]
 
         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")
+        confpy.write(line)
-        api = confpy.read(error=True)
+        error = confpy.read(error=True)
-        error = api.errors[0]
 
-        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

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:
+        if load_autoconfig:
-            config_py_lines.append('config.load_autoconfig = False')
+            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 "
+            error = ("Error{}: Invalid value 'True' - expected a value of "
-                      "type str but got bool.")
+                     "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)