From 0e743f0e0939b2f5d3a783ad11f7ee12cf254dc0 Mon Sep 17 00:00:00 2001 From: Felix Van der Jeugt Date: Tue, 19 Sep 2017 14:08:59 +0200 Subject: [PATCH 1/6] save only a changed autoconfig file --- qutebrowser/config/configfiles.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 3810bca8c..9bad78394 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -25,6 +25,7 @@ import textwrap import traceback import configparser import contextlib +import copy import yaml from PyQt5.QtCore import QSettings @@ -94,7 +95,10 @@ class YamlConfig: save_manager.add_saveable('yaml-config', self._save) def _save(self): - """Save the changed settings to the YAML file.""" + """Save the settings to the YAML file if they've changed.""" + if self.values == self._initial_values: + return + data = {'config_version': self.VERSION, 'global': self.values} with qtutils.savefile_open(self._filename) as f: f.write(textwrap.dedent(""" @@ -106,6 +110,11 @@ class YamlConfig: def load(self): """Load self.values from the configured YAML file.""" + self._initial_values = self._load() + self.values = copy.deepcopy(self._initial_values) + + def _load(self): + """Load configuration from the configured YAML file.""" try: with open(self._filename, 'r', encoding='utf-8') as f: yaml_data = utils.yaml_load(f) @@ -136,7 +145,7 @@ class YamlConfig: "'global' object is not a dict") raise configexc.ConfigFileErrors('autoconfig.yml', [desc]) - self.values = global_obj + return global_obj class ConfigAPI: From 7b192d426ebe95c4f9054f1ffbac41ece0c039ef Mon Sep 17 00:00:00 2001 From: Felix Van der Jeugt Date: Tue, 19 Sep 2017 15:30:28 +0200 Subject: [PATCH 2/6] add unit test and fix issues with it --- qutebrowser/config/configfiles.py | 3 +- tests/unit/config/test_configfiles.py | 40 +++++++++++++++++++++------ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 9bad78394..89c679976 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -85,6 +85,7 @@ class YamlConfig: self._filename = os.path.join(standarddir.config(auto=True), 'autoconfig.yml') self.values = {} + self._initial_values = {} def init_save_manager(self, save_manager): """Make sure the config gets saved properly. @@ -119,7 +120,7 @@ class YamlConfig: with open(self._filename, 'r', encoding='utf-8') as f: yaml_data = utils.yaml_load(f) except FileNotFoundError: - return + return {} except OSError as e: desc = configexc.ConfigErrorDesc("While reading", e) raise configexc.ConfigFileErrors('autoconfig.yml', [desc]) diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index 73b091293..fdf709744 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -72,16 +72,17 @@ def test_yaml_config(fake_save_manager, config_tmpdir, old_config, insert): yaml._save() - text = autoconfig.read_text('utf-8') - lines = text.splitlines() - print(lines) - - assert lines[0].startswith('# DO NOT edit this file by hand,') - assert 'config_version: {}'.format(yaml.VERSION) in lines - - if old_config is None and not insert: - assert 'global: {}' in lines + if not insert and old_config is None: + lines = [] else: + text = autoconfig.read_text('utf-8') + lines = text.splitlines() + print(lines) + + if insert: + assert lines[0].startswith('# DO NOT edit this file by hand,') + assert 'config_version: {}'.format(yaml.VERSION) in lines + assert 'global:' in lines # WORKAROUND for https://github.com/PyCQA/pylint/issues/574 @@ -91,6 +92,27 @@ def test_yaml_config(fake_save_manager, config_tmpdir, old_config, insert): assert ' tabs.show: never' in lines +@pytest.mark.parametrize('old_config', [ + None, + 'global:\n colors.hints.fg: magenta', +]) +def test_yaml_config_unchanged(fake_save_manager, config_tmpdir, old_config): + autoconfig = config_tmpdir / 'autoconfig.yml' + mtime = None + if old_config is not None: + autoconfig.write_text(old_config, 'utf-8') + mtime = autoconfig.stat().mtime + + yaml = configfiles.YamlConfig() + yaml.load() + yaml._save() + + if old_config is None: + assert not autoconfig.exists() + else: + assert autoconfig.stat().mtime == mtime + + @pytest.mark.parametrize('line, text, exception', [ ('%', 'While parsing', 'while scanning a directive'), ('global: 42', 'While loading data', "'global' object is not a dict"), From 8db630d358f6f83f420cf491a5669a29c0ba9365 Mon Sep 17 00:00:00 2001 From: Felix Van der Jeugt Date: Tue, 19 Sep 2017 17:26:03 +0200 Subject: [PATCH 3/6] don't copy values but set dirty --- qutebrowser/config/config.py | 6 +++--- qutebrowser/config/configfiles.py | 30 ++++++++++++++++++--------- tests/helpers/stubs.py | 14 ++++++++++++- tests/unit/config/test_config.py | 22 ++++++++++---------- tests/unit/config/test_configfiles.py | 2 +- 5 files changed, 48 insertions(+), 26 deletions(-) diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index 678417453..316eff2a0 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -408,7 +408,7 @@ class Config(QObject): def read_yaml(self): """Read the YAML settings from self._yaml.""" self._yaml.load() - for name, value in self._yaml.values.items(): + for name, value in self._yaml: self._set_value(self.get_opt(name), value) def get_opt(self, name): @@ -455,7 +455,7 @@ class Config(QObject): """ self._set_value(self.get_opt(name), value) if save_yaml: - self._yaml.values[name] = value + self._yaml[name] = value def set_str(self, name, value, *, save_yaml=False): """Set the given setting from a string. @@ -469,7 +469,7 @@ class Config(QObject): value)) self._set_value(opt, converted) if save_yaml: - self._yaml.values[name] = converted + self._yaml[name] = converted def update_mutables(self, *, save_yaml=False): """Update mutable settings if they changed. diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 89c679976..0e712ea22 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -84,8 +84,8 @@ class YamlConfig: def __init__(self): self._filename = os.path.join(standarddir.config(auto=True), 'autoconfig.yml') - self.values = {} - self._initial_values = {} + self._values = {} + self._changed = None def init_save_manager(self, save_manager): """Make sure the config gets saved properly. @@ -95,12 +95,26 @@ class YamlConfig: """ save_manager.add_saveable('yaml-config', self._save) + def __getitem__(self, name): + return self._values[name] + + def __setitem__(self, name, value): + if self._values.get(name) != value: + self._changed = True + self._values[name] = value + + def __contains__(self, name): + return name in self._values + + def __iter__(self): + return iter(self._values.items()) + def _save(self): """Save the settings to the YAML file if they've changed.""" - if self.values == self._initial_values: + if not self._changed: return - data = {'config_version': self.VERSION, 'global': self.values} + data = {'config_version': self.VERSION, 'global': self._values} with qtutils.savefile_open(self._filename) as f: f.write(textwrap.dedent(""" # DO NOT edit this file by hand, qutebrowser will overwrite it. @@ -110,11 +124,6 @@ class YamlConfig: utils.yaml_dump(data, f) def load(self): - """Load self.values from the configured YAML file.""" - self._initial_values = self._load() - self.values = copy.deepcopy(self._initial_values) - - def _load(self): """Load configuration from the configured YAML file.""" try: with open(self._filename, 'r', encoding='utf-8') as f: @@ -146,7 +155,8 @@ class YamlConfig: "'global' object is not a dict") raise configexc.ConfigFileErrors('autoconfig.yml', [desc]) - return global_obj + self._values = global_obj + self._changed = False class ConfigAPI: diff --git a/tests/helpers/stubs.py b/tests/helpers/stubs.py index d7dcb427f..29fbb28e1 100644 --- a/tests/helpers/stubs.py +++ b/tests/helpers/stubs.py @@ -414,12 +414,24 @@ class FakeYamlConfig: """Fake configfiles.YamlConfig object.""" def __init__(self): - self.values = {} self.loaded = False + self._values = {} def load(self): self.loaded = True + def __contains__(self, item): + return item in self._values + + def __iter__(self): + return iter(self._values.items()) + + def __setitem__(self, key, value): + self._values[key] = value + + def __getitem__(self, key): + return self._values[key] + class StatusBarCommandStub(QLineEdit): diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index 4b86d9a4e..ef0b55d25 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -317,9 +317,9 @@ class TestSetConfigCommand: assert config_stub.get(option) == new_value if temp: - assert option not in config_stub._yaml.values + assert option not in config_stub._yaml else: - assert config_stub._yaml.values[option] == new_value + assert config_stub._yaml[option] == new_value @pytest.mark.parametrize('temp', [True, False]) def test_set_temp_override(self, commands, config_stub, temp): @@ -335,7 +335,7 @@ class TestSetConfigCommand: commands.set(0, 'url.auto_search', 'never', temp=True) assert config_stub.val.url.auto_search == 'never' - assert config_stub._yaml.values['url.auto_search'] == 'dns' + assert config_stub._yaml['url.auto_search'] == 'dns' def test_set_print(self, config_stub, commands, message_mock): """Run ':set -p url.auto_search never'. @@ -357,7 +357,7 @@ class TestSetConfigCommand: assert not config_stub.val.auto_save.session commands.set(0, 'auto_save.session!') assert config_stub.val.auto_save.session - assert config_stub._yaml.values['auto_save.session'] + assert config_stub._yaml['auto_save.session'] def test_set_toggle_nonbool(self, commands, config_stub): """Run ':set url.auto_search!'. @@ -439,7 +439,7 @@ class TestSetConfigCommand: config_stub.set_obj(opt, initial) commands.set(0, opt, 'green', 'magenta', 'blue', 'yellow') assert config_stub.get(opt) == expected - assert config_stub._yaml.values[opt] == expected + assert config_stub._yaml[opt] == expected class TestBindConfigCommand: @@ -464,7 +464,7 @@ class TestBindConfigCommand: commands.bind('a', command) assert keyconf.get_command('a', 'normal') == command - yaml_bindings = config_stub._yaml.values['bindings.commands']['normal'] + yaml_bindings = config_stub._yaml['bindings.commands']['normal'] assert yaml_bindings['a'] == command @pytest.mark.parametrize('key, mode, expected', [ @@ -565,7 +565,7 @@ class TestBindConfigCommand: commands.unbind(key) assert keyconf.get_command(key, 'normal') is None - yaml_bindings = config_stub._yaml.values['bindings.commands']['normal'] + yaml_bindings = config_stub._yaml['bindings.commands']['normal'] if key in 'bc': # Custom binding assert normalized not in yaml_bindings @@ -612,7 +612,7 @@ class TestConfig: def test_read_yaml(self, conf): assert not conf._yaml.loaded - conf._yaml.values['content.plugins'] = True + conf._yaml['content.plugins'] = True conf.read_yaml() @@ -620,7 +620,7 @@ class TestConfig: assert conf._values['content.plugins'] is True def test_read_yaml_invalid(self, conf): - conf._yaml.values['foo.bar'] = True + conf._yaml['foo.bar'] = True with pytest.raises(configexc.NoOptionError): conf.read_yaml() @@ -743,9 +743,9 @@ class TestConfig: meth(option, value, save_yaml=save_yaml) assert conf._values[option] is True if save_yaml: - assert conf._yaml.values[option] is True + assert conf._yaml[option] is True else: - assert option not in conf._yaml.values + assert option not in conf._yaml @pytest.mark.parametrize('method', ['set_obj', 'set_str']) def test_set_invalid(self, conf, qtbot, method): diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index fdf709744..3680cf6b4 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -68,7 +68,7 @@ def test_yaml_config(fake_save_manager, config_tmpdir, old_config, insert): yaml.load() if insert: - yaml.values['tabs.show'] = 'never' + yaml['tabs.show'] = 'never' yaml._save() From 8e14d1b7e6c77e58cbe7fda42875da8e81202149 Mon Sep 17 00:00:00 2001 From: Felix Van der Jeugt Date: Tue, 19 Sep 2017 17:47:38 +0200 Subject: [PATCH 4/6] remove unused import --- qutebrowser/config/configfiles.py | 1 - 1 file changed, 1 deletion(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 0e712ea22..b187a6c08 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -25,7 +25,6 @@ import textwrap import traceback import configparser import contextlib -import copy import yaml from PyQt5.QtCore import QSettings From 285b5343846929f38ac1284a6c9daee4847f4596 Mon Sep 17 00:00:00 2001 From: Felix Van der Jeugt Date: Wed, 20 Sep 2017 10:04:34 +0200 Subject: [PATCH 5/6] make changed dirty and save on duplicate write --- qutebrowser/config/configfiles.py | 9 ++++----- tests/unit/config/test_configfiles.py | 3 ++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index b187a6c08..06c3931f6 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -84,7 +84,7 @@ class YamlConfig: self._filename = os.path.join(standarddir.config(auto=True), 'autoconfig.yml') self._values = {} - self._changed = None + self._dirty = None def init_save_manager(self, save_manager): """Make sure the config gets saved properly. @@ -98,8 +98,7 @@ class YamlConfig: return self._values[name] def __setitem__(self, name, value): - if self._values.get(name) != value: - self._changed = True + self._dirty = True self._values[name] = value def __contains__(self, name): @@ -110,7 +109,7 @@ class YamlConfig: def _save(self): """Save the settings to the YAML file if they've changed.""" - if not self._changed: + if not self._dirty: return data = {'config_version': self.VERSION, 'global': self._values} @@ -155,7 +154,7 @@ class YamlConfig: raise configexc.ConfigFileErrors('autoconfig.yml', [desc]) self._values = global_obj - self._changed = False + self._dirty = False class ConfigAPI: diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index 3680cf6b4..5736904c2 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -77,7 +77,6 @@ def test_yaml_config(fake_save_manager, config_tmpdir, old_config, insert): else: text = autoconfig.read_text('utf-8') lines = text.splitlines() - print(lines) if insert: assert lines[0].startswith('# DO NOT edit this file by hand,') @@ -85,6 +84,8 @@ def test_yaml_config(fake_save_manager, config_tmpdir, old_config, insert): assert 'global:' in lines + print(lines) + # WORKAROUND for https://github.com/PyCQA/pylint/issues/574 if 'magenta' in (old_config or ''): # pylint: disable=superfluous-parens assert ' colors.hints.fg: magenta' in lines From 6892705e186462cde399a55b3fc047516c73159a Mon Sep 17 00:00:00 2001 From: Felix Van der Jeugt Date: Wed, 20 Sep 2017 15:52:42 +0200 Subject: [PATCH 6/6] cover setting-saving-loading-getting yaml config --- tests/unit/config/test_configfiles.py | 31 +++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index 5736904c2..e252762b6 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -93,6 +93,37 @@ def test_yaml_config(fake_save_manager, config_tmpdir, old_config, insert): assert ' tabs.show: never' in lines +@pytest.mark.parametrize('old_config', [ + None, + 'global:\n colors.hints.fg: magenta', +]) +@pytest.mark.parametrize('key, value', [ + ('colors.hints.fg', 'green'), + ('colors.hints.bg', None), + ('confirm_quit', True), + ('confirm_quit', False), +]) +def test_yaml_config_changed(fake_save_manager, config_tmpdir, old_config, key, value): + autoconfig = config_tmpdir / 'autoconfig.yml' + if old_config is not None: + autoconfig.write_text(old_config, 'utf-8') + + yaml = configfiles.YamlConfig() + yaml.load() + + yaml[key] = value + assert key in yaml + assert yaml[key] == value + + yaml._save() + + yaml = configfiles.YamlConfig() + yaml.load() + + assert key in yaml + assert yaml[key] == value + + @pytest.mark.parametrize('old_config', [ None, 'global:\n colors.hints.fg: magenta',