Add support for positional arguments

argparse makes it awkward to implement the current cfg API where we
simply return the leftover arguments to the caller.

Very few cfg users actually rely on this functionality and for those
cases it probably makes more sense for them to explicitly register
positional arguments or sub-parsers.

Add support for positional arguments via a 'required' Opt attribute:

  opt = StrOpt('foo', positional=True)
  conf.register_cli_opt(opt)
  conf(['bar'])
  conf.foo == 'bar'

Change-Id: Iea746d710237e1ea26c1ef4871643941d1df09bd

2 files changed, 113 insertions, 51 deletions

diff --git a/openstack/common/cfg.py b/openstack/common/cfg.py
index 7939d02..2430077 100644
--- a/openstack/common/cfg.py
+++ b/openstack/common/cfg.py
@@ -475,6 +475,8 @@ class Opt(object):
         a single character CLI option name
       default:
         the default value of the option
+      positional:
+        True if the option is a positional CLI argument
       metavar:
         the name shown as the argument to a CLI option in --help output
       help:
@@ -483,8 +485,8 @@ class Opt(object):
     multi = False
 
     def __init__(self, name, dest=None, short=None, default=None,
-                 metavar=None, help=None, secret=False, required=False,
-                 deprecated_name=None):
+                 positional=False, metavar=None, help=None,
+                 secret=False, required=False, deprecated_name=None):
         """Construct an Opt object.
 
         The only required parameter is the option's name. However, it is
@@ -494,6 +496,7 @@ class Opt(object):
         :param dest: the name of the corresponding ConfigOpts property
         :param short: a single character CLI option name
         :param default: the default value of the option
+        :param positional: True if the option is a positional CLI argument
         :param metavar: the option argument to show in --help
         :param help: an explanation of how the option is used
         :param secret: true iff the value should be obfuscated in log output
@@ -507,6 +510,7 @@ class Opt(object):
             self.dest = dest
         self.short = short
         self.default = default
+        self.positional = positional
         self.metavar = metavar
         self.help = help
         self.secret = secret
@@ -551,10 +555,10 @@ class Opt(object):
         kwargs = self._get_argparse_kwargs(group)
         prefix = self._get_argparse_prefix('', group)
         self._add_to_argparse(container, self.name, self.short, kwargs, prefix,
-                              self.deprecated_name)
+                              self.positional, self.deprecated_name)
 
     def _add_to_argparse(self, container, name, short, kwargs, prefix='',
-                         deprecated_name=None):
+                         positional=False, deprecated_name=None):
         """Add an option to an argparse parser or group.
 
         :param container: an argparse._ArgumentGroup object
@@ -562,14 +566,17 @@ class Opt(object):
         :param short: the short opt name
         :param kwargs: the keyword arguments for add_argument()
         :param prefix: an optional prefix to prepend to the opt name
+        :param position: whether the optional is a positional CLI argument
         :raises: DuplicateOptError if a naming confict is detected
         """
-        args = ['--' + prefix + name]
-        if short:
-            args += ['-' + short]
+        def hyphen(arg):
+            return arg if not positional else ''
 
+        args = [hyphen('--') + prefix + name]
+        if short:
+            args.append(hyphen('-') + short)
         if deprecated_name:
-            args += ['--' + prefix + deprecated_name]
+            args.append(hyphen('--') + prefix + deprecated_name)
 
         try:
             container.add_argument(*args, **kwargs)
@@ -598,11 +605,14 @@ class Opt(object):
         :param kwargs: optional keyword arguments to add to
         :returns: a dict of keyword arguments
         """
-        dest = self.dest
-        if group is not None:
-            dest = group.name + '_' + dest
-        kwargs.update({'dest': dest,
-                       'metavar': self.metavar,
+        if not self.positional:
+            dest = self.dest
+            if group is not None:
+                dest = group.name + '_' + dest
+            kwargs['dest'] = dest
+        else:
+            kwargs['nargs'] = '?'
+        kwargs.update({'metavar': self.metavar,
                        'help': self.help, })
         return kwargs
 
@@ -644,6 +654,11 @@ class BoolOpt(Opt):
     _boolean_states = {'1': True, 'yes': True, 'true': True, 'on': True,
                        '0': False, 'no': False, 'false': False, 'off': False}
 
+    def __init__(self, *args, **kwargs):
+        if 'positional' in kwargs:
+            raise ValueError('positional boolean args not supported')
+        super(BoolOpt, self).__init__(*args, **kwargs)
+
     def _get_from_config_parser(self, cparser, section):
         """Retrieve the opt value as a boolean from ConfigParser."""
         def convert_bool(v):
@@ -668,7 +683,7 @@ class BoolOpt(Opt):
         prefix = self._get_argparse_prefix('no', group)
         kwargs["help"] = "The inverse of --" + self.name
         self._add_to_argparse(container, self.name, None, kwargs, prefix,
-                              self.deprecated_name)
+                              self.positional, self.deprecated_name)
 
     def _get_argparse_kwargs(self, group, action='store_true', **kwargs):
         """Extends the base argparse keyword dict for boolean options."""
@@ -733,16 +748,20 @@ class ListOpt(Opt):
                      default=None,
                      required=False,
                      help=None,
-                     metavar=None):
+                     metavar=None,
+                     nargs=None):
             argparse._StoreAction.__init__(self,
                                            option_strings=option_strings,
                                            dest=dest,
                                            default=default,
                                            required=required,
-                                           help=help)
+                                           help=help,
+                                           nargs=nargs)
 
         def __call__(self, parser, namespace, values, option_string=None):
-            setattr(namespace, self.dest, values.split(','))
+            if values is not None:
+                values = values.split(',')
+            setattr(namespace, self.dest, values)
 
     def _get_from_config_parser(self, cparser, section):
         """Retrieve the opt value as a list from ConfigParser."""
@@ -767,8 +786,12 @@ class MultiStrOpt(Opt):
 
     def _get_argparse_kwargs(self, group, **kwargs):
         """Extends the base argparse keyword dict for multi str options."""
-        return super(MultiStrOpt,
-                     self)._get_argparse_kwargs(group, action='append')
+        kwargs = super(MultiStrOpt, self)._get_argparse_kwargs(group)
+        if not self.positional:
+            kwargs['action'] = 'append'
+        else:
+            kwargs['nargs'] = '*'
+        return kwargs
 
     def _cparser_get_with_deprecated(self, cparser, section):
         """If cannot find option as dest try deprecated_name alias."""
@@ -942,20 +965,6 @@ class ConfigCliParser(argparse.ArgumentParser):
     def add_subparsers(self, **kwargs):
         return super(ConfigCliParser, self).add_subparsers(**kwargs)
 
-    def parse_args(self, *args, **kwargs):
-        opts, args = super(ConfigCliParser, self).parse_known_args(*args,
-                                                                   **kwargs)
-        unknown_args = []
-        for arg in args:
-            if 0 == arg.find("-"):
-                unknown_args.append(arg)
-
-        if unknown_args:
-            msg = 'unrecognized arguments: %s'
-            self.error(msg % ' '.join(unknown_args))
-
-        return opts, args
-
 
 class ConfigOpts(collections.Mapping):
 
@@ -1090,14 +1099,12 @@ class ConfigOpts(collections.Mapping):
 
         self._setup(project, prog, version, usage, default_config_files)
 
-        self._cli_values, leftovers = self._parse_cli_opts(args)
+        self._cli_values = self._parse_cli_opts(args)
 
         self._parse_config_files()
 
         self._check_required_opts()
 
-        return leftovers
-
     def __getattr__(self, name):
         """Look up an option value and perform string substitution.
 
@@ -1452,6 +1459,10 @@ class ConfigOpts(collections.Mapping):
             if not opt.multi:
                 return value
 
+            # argparse ignores default=None for nargs='*'
+            if opt.positional and not value:
+                value = opt.default
+
             return value + values
 
         if values:
@@ -1577,9 +1588,7 @@ class ConfigOpts(collections.Mapping):
         for opt, group in self._all_opts():
             opt._add_to_cli(self._oparser, group)
 
-        values, leftovers = self._oparser.parse_args(args)
-
-        return vars(values), leftovers
+        return vars(self._oparser.parse_args(args))
 
     class GroupAttr(collections.Mapping):
 
diff --git a/tests/unit/test_cfg.py b/tests/unit/test_cfg.py
index 4289c7d..384a57a 100644
--- a/tests/unit/test_cfg.py
+++ b/tests/unit/test_cfg.py
@@ -137,17 +137,6 @@ class HelpTestCase(BaseTestCase):
         self.assertTrue('-h, --help' in f.getvalue())
 
 
-class LeftoversTestCase(BaseTestCase):
-
-    def test_leftovers(self):
-        self.conf.register_cli_opts([StrOpt('foo'), StrOpt('bar')])
-
-        leftovers = self.conf(['those', '--foo', 'this',
-                               'thems', '--bar', 'that', 'these'])
-
-        self.assertEquals(leftovers, ['those', 'thems', 'these'])
-
-
 class FindConfigFilesTestCase(BaseTestCase):
 
     def test_find_config_files(self):
@@ -268,6 +257,70 @@ class CliOptsTestCase(BaseTestCase):
         self.assertEquals(self.conf.config_file, paths)
 
 
+class PositionalTestCase(BaseTestCase):
+
+    def _do_pos_test(self, opt_class, default, cli_args, value):
+        self.conf.register_cli_opt(opt_class('foo',
+                                             default=default,
+                                             positional=True))
+
+        self.conf(cli_args)
+
+        self.assertTrue(hasattr(self.conf, 'foo'))
+        self.assertEquals(self.conf.foo, value)
+
+    def test_positional_str_default(self):
+        self._do_pos_test(StrOpt, None, [], None)
+
+    def test_positional_str_arg(self):
+        self._do_pos_test(StrOpt, None, ['bar'], 'bar')
+
+    def test_positional_int_default(self):
+        self._do_pos_test(IntOpt, 10, [], 10)
+
+    def test_positional_int_arg(self):
+        self._do_pos_test(IntOpt, None, ['20'], 20)
+
+    def test_positional_float_default(self):
+        self._do_pos_test(FloatOpt, 1.0, [], 1.0)
+
+    def test_positional_float_arg(self):
+        self._do_pos_test(FloatOpt, None, ['2.0'], 2.0)
+
+    def test_positional_list_default(self):
+        self._do_pos_test(ListOpt, ['bar'], [], ['bar'])
+
+    def test_positional_list_arg(self):
+        self._do_pos_test(ListOpt, None,
+                          ['blaa,bar'], ['blaa', 'bar'])
+
+    def test_positional_multistr_default(self):
+        self._do_pos_test(MultiStrOpt, ['bar'], [], ['bar'])
+
+    def test_positional_multistr_arg(self):
+        self._do_pos_test(MultiStrOpt, None,
+                          ['blaa', 'bar'], ['blaa', 'bar'])
+
+    def test_positional_bool(self):
+        self.assertRaises(ValueError, BoolOpt, 'foo', positional=True)
+
+    def test_required_positional_opt(self):
+        self.conf.register_cli_opt(StrOpt('foo',
+                                          required=True,
+                                          positional=True))
+
+        self.conf(['bar'])
+
+        self.assertTrue(hasattr(self.conf, 'foo'))
+        self.assertEquals(self.conf.foo, 'bar')
+
+    def test_missing_required_cli_opt(self):
+        self.conf.register_cli_opt(StrOpt('foo',
+                                          required=True,
+                                          positional=True))
+        self.assertRaises(RequiredOptError, self.conf, [])
+
+
 class ConfigFileOptsTestCase(BaseTestCase):
 
     def _do_deprecated_test_use(self, opt_class, value, result):
@@ -387,7 +440,7 @@ class ConfigFileOptsTestCase(BaseTestCase):
                                         '[DEFAULT]\n'
                                         'foo = yes\n')])
 
-        self.conf(['--foo', 'bar',
+        self.conf(['--foo',
                    '--config-file', paths[0],
                    '--config-file', paths[1]])