Merge pull request #89 from larsbutler/fix-config-precedence

fix(config): fix order of precedence in option processing

simpl/config.py

@@ -477,8 +477,15 @@ def parse_cli(self, argv=None, permissive=False):
         options = []
         for option in self._options:
             kwargs = option.kwargs.copy()
-            if kwargs.get('default') is None:
+            original_default = kwargs.get('default')
+            if original_default is not None:
                 kwargs['default'] = argparse.SUPPRESS
+                # Since we're suppressing defaults, we need to somehow preserve
+                # the default value for the help text:
+                help_text = kwargs.get('help', '')
+                kwargs['help'] = '%s (default: %s)' % (
+                    help_text, original_default
+                )
             temp = Option(*option.args, **kwargs)
             options.append(temp)
         parser = self.build_parser(options, permissive=permissive)
@@ -615,11 +622,18 @@ def load_options(self, argv=None, keyring_namespace=None, permissive=True):
         secrets = self.parse_keyring(keyring_namespace)
         ini = self.parse_ini()
 
+        drop_nones = lambda d: {key: value for key, value in d.items()
+                                if value is not None}
+        # NOTE(larsbutler): We want to filter out nones, so that options with a
+        # None value at a higher precedence do not override values with a
+        # non-None value at a lower precedence. For example, if the defaults
+        # define a value for the option `foo`, and `args` contains `foo=None`,
+        # the `args` should override the default.
         results = defaults
-        results.update(ini)
-        results.update(secrets)
-        results.update(env)
-        results.update(args)
+        results.update(drop_nones(ini))
+        results.update(drop_nones(secrets))
+        results.update(drop_nones(env))
+        results.update(drop_nones(args))
         return results
 
     def parse(self, argv=None, keyring_namespace=None, strict=False):

tests/test_config.py

@@ -430,5 +430,63 @@ def test_default_help_formatter(self, mock_stdout):
 """
         self.assertEqual(mock_stdout.getvalue(), expected)
 
+
+class TestConfigPrecedence(unittest.TestCase):
+
+    def setUp(self):
+        self.opts = [
+            config.Option(
+                '--foo',
+                required=True,
+                default='bar',
+                env='FOO',
+            ),
+            config.Option(
+                '--blarg',
+                required=True,
+                default='baz',
+                env='BLARG',
+            ),
+        ]
+
+        self.conf = config.Config(
+            options=self.opts,
+            prog='test',
+        )
+
+    @mock.patch.dict(config.os.environ, {})
+    def test_case_1(self):
+        # Test that defaults are used if no env or cli source of the option are
+        # present.
+        res = self.conf.parse(argv=['test.py'])
+        self.assertEqual(res.foo, 'bar')
+        self.assertEqual(res.blarg, 'baz')
+
+    @mock.patch.dict(config.os.environ, {})
+    def test_case_2(self):
+        # Test that cli args override the defaults.
+        res = self.conf.parse(argv=['test.py', '--foo', 'fromcli'])
+        self.assertEqual(res.foo, 'fromcli')
+        self.assertEqual(res.blarg, 'baz')
+
+    @mock.patch.dict(config.os.environ, {'FOO': 'fromenv'})
+    def test_case_3(self):
+        # Test that defaults are overridden by the environment
+        # _if_ the env var is present.
+        res = self.conf.parse(argv=['test.py'])
+        self.assertEqual(res.foo, 'fromenv')
+        # Option --blarg can also be set by the envvar BLARG, but it is not
+        # present. Thus, we expect to get the default again:
+        self.assertEqual(res.blarg, 'baz')
+
+    @mock.patch.dict(config.os.environ, {'FOO': 'fromenv'})
+    def test_case_4(self):
+        # Similar to above, but now the cli args will override both the
+        # default and then the env (in that order).
+        res = self.conf.parse(argv=['test.py', '--foo', 'fromcli'])
+        self.assertEqual(res.foo, 'fromcli')
+        self.assertEqual(res.blarg, 'baz')
+
+
 if __name__ == '__main__':
     unittest.main()