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

Default values were overriding values set at higher levels of
precedence, namely command line args (which have the highest
precedence!).

Patch includes specific tests cases, which could be expanded to include
ini and keyring sources of config options. (The tests only cover
defaults, env, and command line sources of config params.)

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