From e20dfcd3eb76664360a03b18da78c710e094ffb7 Mon Sep 17 00:00:00 2001 From: Neil Booth Date: Mon, 25 May 2015 12:13:58 +0900 Subject: [PATCH] Fix SimpleConfig SimpleConfig claims to handle configuration options in priority command line, user config, system config, which makes sense. In fact it appears it used priority command line, system config, user config. Also, from the priority ordering, it would seem correct that a value should be unmodifiable if and only if it's set on the command line. Previously anything in the system config would be unmodifiable. This patch fixes these and cleans the code up a bit. I noticed this whilst attempting to unify the 'auto_cycle' setting. Fixup tests accordingly. --- lib/simple_config.py | 71 +++++++++++++-------------------- lib/tests/test_simple_config.py | 16 ++++---- 2 files changed, 35 insertions(+), 52 deletions(-) diff --git a/lib/simple_config.py b/lib/simple_config.py index 02280d1f..71d8d718 100644 --- a/lib/simple_config.py +++ b/lib/simple_config.py @@ -3,6 +3,7 @@ import json import threading import os +from copy import deepcopy from util import user_dir, print_error, print_msg SYSTEM_CONFIG_PATH = "/etc/electrum.conf" @@ -32,19 +33,12 @@ class SimpleConfig(object): They are taken in order (1. overrides config options set in 2., that override config set in 3.) """ - def __init__(self, options=None, read_system_config_function=None, + def __init__(self, options={}, read_system_config_function=None, read_user_config_function=None, read_user_dir_function=None): - # This is the holder of actual options for the current user. - self.read_only_options = {} # This lock needs to be acquired for updating and reading the config in # a thread-safe way. self.lock = threading.RLock() - # The path for the config directory. This is set later by init_path() - self.path = None - - if options is None: - options = {} # Having a mutable as a default value is a bad idea. # The following two functions are there for dependency injection when # testing. @@ -57,45 +51,39 @@ class SimpleConfig(object): else: self.user_dir = read_user_dir_function - # Save the command-line keys to make sure we don't override them. - self.command_line_keys = options.keys() - # Save the system config keys to make sure we don't override them. - self.system_config_keys = [] + # The command line options + self.cmdline_options = deepcopy(options) - if options.get('portable') is not True: - # system conf - system_config = read_system_config_function() - self.system_config_keys = system_config.keys() - self.read_only_options.update(system_config) + # Portable wallets don't use a system config + if self.cmdline_options.get('portable', False): + self.system_config = read_system_config_function() + else: + self.system_config = {} - # update the current options with the command line options last (to - # override both others). - self.read_only_options.update(options) - # init path - self.init_path() - # user config. + # Set self.path and read the user config + self.user_config = {} # for self.get in electrum_path() + self.path = self.electrum_path() self.user_config = read_user_config_function(self.path) # Make a singleton instance of 'self' set_config(self) - def init_path(self): - # Read electrum path in the command line configuration - self.path = self.read_only_options.get('electrum_path') - - # If not set, use the user's default data directory. - if self.path is None: - self.path = self.user_dir() + def electrum_path(self): + # Read electrum_path from command line / system configuration + # Otherwise use the user's default data directory. + path = self.get('electrum_path') + if path is None: + path = self.user_dir() # Make directory if it does not yet exist. - if not os.path.exists(self.path): - os.mkdir(self.path) + if not os.path.exists(path): + os.mkdir(path) - print_error( "electrum directory", self.path) + print_error("electrum directory", path) + return path def set_key(self, key, value, save = True): if not self.is_modifiable(key): - print "Warning: not changing key '%s' because it is not modifiable" \ - " (passed as command line option or defined in /etc/electrum.conf)"%key + print_error("Warning: not changing config key '%s' set on the command line" % key) return with self.lock: @@ -105,19 +93,16 @@ class SimpleConfig(object): return def get(self, key, default=None): - out = None with self.lock: - out = self.read_only_options.get(key) + out = self.cmdline_options.get(key) if out is None: - out = self.user_config.get(key, default) + out = self.user_config.get(key) + if out is None: + out = self.system_config.get(key, default) return out def is_modifiable(self, key): - if key in self.command_line_keys: - return False - if key in self.system_config_keys: - return False - return True + return not key in self.cmdline_options def save_user_config(self): if not self.path: diff --git a/lib/tests/test_simple_config.py b/lib/tests/test_simple_config.py index a800629d..339b756d 100644 --- a/lib/tests/test_simple_config.py +++ b/lib/tests/test_simple_config.py @@ -48,17 +48,16 @@ class Test_SimpleConfig(unittest.TestCase): self.assertEqual(self.options.get("electrum_path"), config.get("electrum_path")) - def test_simple_config_system_config_overrides_user_config(self): - """Options passed in system config override user config.""" + def test_simple_config_user_config_overrides_system_config(self): + """Options passed in user config override system config.""" fake_read_system = lambda : {"electrum_path": self.electrum_dir} fake_read_user = lambda _: {"electrum_path": "b"} read_user_dir = lambda : self.user_dir - config = SimpleConfig(options=None, + config = SimpleConfig(options={}, read_system_config_function=fake_read_system, read_user_config_function=fake_read_user, read_user_dir_function=read_user_dir) - self.assertEqual(self.options.get("electrum_path"), - config.get("electrum_path")) + self.assertEqual("b", config.get("electrum_path")) def test_simple_config_system_config_ignored_if_portable(self): """If electrum is started with the "portable" flag, system @@ -79,7 +78,7 @@ class Test_SimpleConfig(unittest.TestCase): fake_read_system = lambda : {} fake_read_user = lambda _: {"electrum_path": self.electrum_dir} read_user_dir = lambda : self.user_dir - config = SimpleConfig(options=None, + config = SimpleConfig(options={}, read_system_config_function=fake_read_system, read_user_config_function=fake_read_user, read_user_dir_function=read_user_dir) @@ -98,7 +97,7 @@ class Test_SimpleConfig(unittest.TestCase): self.assertEqual(self.options.get("electrum_path"), config.get("electrum_path")) - def test_cannot_set_options_from_system_config(self): + def test_can_set_options_from_system_config(self): fake_read_system = lambda : {"electrum_path": self.electrum_dir} fake_read_user = lambda _: {} read_user_dir = lambda : self.user_dir @@ -107,8 +106,7 @@ class Test_SimpleConfig(unittest.TestCase): read_user_config_function=fake_read_user, read_user_dir_function=read_user_dir) config.set_key("electrum_path", "c") - self.assertEqual(self.options.get("electrum_path"), - config.get("electrum_path")) + self.assertEqual("c", config.get("electrum_path")) def test_can_set_options_set_in_user_config(self): another_path = tempfile.mkdtemp()