New config: More powerful :config- commands: add #4283

Made requested changes:

- Separated list add and dict add commands.
- Separated list and dict completion models.
- Created tests for each command.
- Simplified the configmodel options by breaking them into a separate
function to do work that is similar.
- General simplification of both add commands.

Continues #2794

qutebrowser/completion/models/configmodel.py

@@ -27,12 +27,7 @@ from qutebrowser.keyinput import keyutils
 
 def option(*, info):
     """A CompletionModel filled with settings and their descriptions."""
-    model = completionmodel.CompletionModel(column_widths=(20, 70, 10))
+    return _option(info, "Options", lambda opt: not opt.no_autoconfig)
-    options = ((opt.name, opt.description, info.config.get_str(opt.name))
-               for opt in configdata.DATA.values()
-               if not opt.no_autoconfig)
-    model.add_category(listcategory.ListCategory("Options", options))
-    return model
 
 
 def customized_option(*, info):
@@ -47,18 +42,32 @@ def customized_option(*, info):
     return model
 
 
-def structure_option(*, info):
+def list_option(*, info):
-    """A CompletionModel filled with set settings of structures.
+    """A CompletionModel filled with settings whose values are lists."""
+    predicate = lambda opt: isinstance(info.config.get_obj(opt.name), list)
+    return _option(info, "List options", predicate)
 
-    Gets lists and dicts and their descriptions.
+
+def dict_option(*, info):
+    """A CompletionModel filled with settings whose values are dicts."""
+    predicate = lambda opt: isinstance(info.config.get_obj(opt.name), dict)
+    return _option(info, "Dict options", predicate)
+
+
+def _option(info, title, predicate):
+    """A CompletionModel that is generified for several option sets.
+
+    Args:
+        info: The config info that can be passed through.
+        title: The title of the options.
+        predicate: The function for filtering out the options. Takes a single
+            argument.
     """
     model = completionmodel.CompletionModel(column_widths=(20, 70, 10))
     options = ((opt.name, opt.description, info.config.get_str(opt.name))
                for opt in configdata.DATA.values()
-               if isinstance(info.config.get_obj(opt.name), (list, dict)))
+               if predicate(opt))
-    model.add_category(
+    model.add_category(listcategory.ListCategory(title, options))
-        listcategory.ListCategory("Structure options", options)
-    )
     return model
 
 

qutebrowser/config/configcommands.py

@@ -251,70 +251,58 @@ class ConfigCommands:
             self._config.unset(option, save_yaml=not temp)
 
     @cmdutils.register(instance='config-commands')
-    @cmdutils.argument('option', completion=configmodel.structure_option)
+    @cmdutils.argument('option', completion=configmodel.list_option)
-    def config_add(
+    def config_add_list(self, option, value, temp=False):
-            self, option, key=None, value=None, temp=False, replace=False
+        """Append a value to a config option that is a list.
-    ):
-        """Adds an option.
 
-        This adds an element to a dictionary or list. --replace is needed
+        This appends an option to a config setting that is a list.
-        to override existing values.
 
         Args:
             option: The name of the option.
-            value: The value to place in either the list or the dictionary.
+            value: The value to append to the end of the dictionary.
-            key: The key to set if it's a dictionary.
+            temp: Don't touch autoconfig.yml.
+        """
+        opt = self._config.get_opt(option)
+        valid_list_types = (configtypes.List, configtypes.ListOrValue)
+        if not isinstance(opt.typ, valid_list_types):
+            raise cmdexc.CommandError(":config-add-list can only be used for "
+                                      "lists")
+
+        with self._handle_config_error():
+            option_value = self._config.get_mutable_obj(option)
+            option_value.append(value)
+            self._config.update_mutables(save_yaml=not temp)
+
+    @cmdutils.register(instance='config-commands')
+    @cmdutils.argument('option', completion=configmodel.dict_option)
+    def config_add_dict(self, option, key, value, temp=False, replace=False):
+        """Add a value at the key within the option specified.
+
+        This adds an element to a dictionary. --replace is needed to override
+        existing values.
+
+        Args:
+            option: The name of the option.
+            key: The key to use.
+            value: The value to place in the dictionary.
             temp: Don't touch autoconfig.yml.
             replace: Whether or not we should replace, default is not.
         """
+        opt = self._config.get_opt(option)
+        if not isinstance(opt.typ, configtypes.Dict):
+            raise cmdexc.CommandError(":config-add-list can only be used for "
+                                      "dicts")
+
         with self._handle_config_error():
-            option_value = self._config.get_obj(option)
+            option_value = self._config.get_mutable_obj(option)
 
-            # Attempt to replace if it's there.
+            if key in option_value and not replace:
-            if isinstance(option_value, list):
+                raise cmdexc.CommandError(("{} already existed in {} - use "
-                if value is None:
+                                           "--replace to overwrite!")
-                    # In this case our key is teh "value", since we are just
+                                          .format(key, option))
-                    # adding to the list.
-                    option_value.append(key)
 
-                if value and replace:
+            option_value[key] = value
-                    # In this case we are trying to replace something at an
+            self._config.update_mutables(save_yaml=not temp)
-                    # index.
-                    try:
-                        index = int(key)
-                        if index >= len(option_value):
-                            option_value.append(value)
-                        else:
-                            option_value[int(key)] = value
-                    except:
-                        raise cmdexc.CommandError("The index must be a number")
-                else:
-                    raise cmdexc.CommandError(
-                        "Use --replace to replace a value at an index")
-            elif isinstance(option_value, dict):
-                # Here we are trying to add something to a dictionary.
-                if value is None and not replace:
-                    raise cmdexc.CommandError((
-                        "{value} should not be empty unless --replace is"
-                        "provided (in which case a 'destroy' will happen)!"
-                    ).format(value=value))
-
-                if not replace and key in option_value:
-                    raise cmdexc.CommandError((
-                        "{key} already existed in {option} - use --replace"
-                        "to overwrite!"
-                    ).format(key=key, option=option))
-
-                # If we made it this far with a None value, we should destroy
-                # the option in the dictionary, otherwise we should insert it.
-                if value is None:
-                    option_value.pop(key, None)
-                else:
-                    option_value[key] = value
-
-            # Set the option in the config.
-            self._config.set_obj(option, option_value, pattern=None,
-                                 save_yaml=not temp)
 
     @cmdutils.register(instance='config-commands')
     def config_clear(self, save=False):

tests/unit/config/test_configcommands.py

@@ -25,7 +25,7 @@ import unittest.mock
 import pytest
 from PyQt5.QtCore import QUrl
 
-from qutebrowser.config import configcommands, configutils
+from qutebrowser.config import configcommands, configtypes, configutils
 from qutebrowser.commands import cmdexc
 from qutebrowser.utils import usertypes, urlmatch
 from qutebrowser.keyinput import keyutils
@@ -282,6 +282,54 @@ class TestCycle:
         assert msg.text == 'auto_save.session = true'
 
 
+class TestAdd:
+
+    """Test :config-add-list and :config-add-dict."""
+
+    @pytest.mark.parametrize('name', ['content.host_blocking.whitelist',
+                                      'history_gap_interval'])
+    @pytest.mark.parametrize('temp', [True, False])
+    @pytest.mark.parametrize('value', ['test1', 'test2', '', None])
+    def test_add_list(self, commands, config_stub, yaml_value, name, temp, value):
+        opt_type = config_stub.get_opt(name).typ
+
+        try:
+            commands.config_add_list(name, value, temp=temp)
+        except cmdexc.CommandError:
+            # We attempted to add to the dictionary with replace as false.
+            valid_list_types = (configtypes.List, configtypes.ListOrValue)
+            assert not isinstance(opt_type, valid_list_types) or not value
+            return
+
+        assert str(config_stub.get(name)[-1]) == value
+        if temp:
+            assert yaml_value(name) == configutils.UNSET
+        else:
+            assert yaml_value(name)[-1] == value
+
+    @pytest.mark.parametrize('name', ['aliases', 'history_gap_interval'])
+    @pytest.mark.parametrize('key', ['w', 'missingkey'])
+    @pytest.mark.parametrize('value', ['test1', 'test2'])
+    @pytest.mark.parametrize('temp', [True, False])
+    @pytest.mark.parametrize('replace', [True, False])
+    def test_add_dict(self, commands, config_stub, yaml_value, name, key,
+                      value, temp, replace):
+        opt_type = config_stub.get_opt(name).typ
+
+        try:
+            commands.config_add_dict(name, key, value, temp=temp, replace=replace)
+        except cmdexc.CommandError:
+            # We attempted to add to the dictionary with replace as false.
+            assert not isinstance(opt_type, configtypes.Dict) or not replace
+            return
+
+        assert str(config_stub.get(name)[key]) == value
+        if temp:
+            assert yaml_value(name) == configutils.UNSET
+        else:
+            assert yaml_value(name)[key] == value
+
+
 class TestUnsetAndClear:
 
     """Test :config-unset and :config-clear."""