Merge "Assume positional arguments are required"

doc/source/reference/command-line.rst

@@ -17,6 +17,10 @@ constructor argument:
     >>> conf.bar
     ['a', 'b']
 
+By default, positional arguments are also required. You may opt-out of this
+behavior by setting ``required=False``, to have an optional positional
+argument.
+
 Sub-Parsers
 -----------
 

oslo_config/cfg.py

@@ -540,7 +540,7 @@ class Opt(object):
 
     def __init__(self, name, type=None, dest=None, short=None,
                  default=None, positional=False, metavar=None, help=None,
-                 secret=False, required=False,
+                 secret=False, required=None,
                  deprecated_name=None, deprecated_group=None,
                  deprecated_opts=None, sample_default=None,
                  deprecated_for_removal=False, deprecated_reason=None,
@@ -556,6 +556,11 @@ def __init__(self, name, type=None, dest=None, short=None,
             raise TypeError('type must be callable')
         self.type = type
 
+        # By default, non-positional options are *optional*, and positional
+        # options are *required*.
+        if required is None:
+            required = True if positional else False
+
         if dest is None:
             self.dest = self.name.replace('-', '_')
         else:
@@ -751,7 +756,7 @@ def _get_argparse_kwargs(self, group, **kwargs):
             if group is not None:
                 dest = group.name + '_' + dest
             kwargs['dest'] = dest
-        else:
+        elif not self.required:
             kwargs['nargs'] = '?'
         kwargs.update({'default': None,
                        'metavar': self.metavar,

oslo_config/tests/test_cfg.py

@@ -234,7 +234,8 @@ def test_print_sorted_help(self):
 
     def test_print_sorted_help_with_positionals(self):
         f = moves.StringIO()
-        self.conf.register_cli_opt(cfg.StrOpt('pst', positional=True))
+        self.conf.register_cli_opt(
+            cfg.StrOpt('pst', positional=True, required=False))
         self.conf.register_cli_opt(cfg.StrOpt('abc'))
         self.conf.register_cli_opt(cfg.StrOpt('zba'))
         self.conf.register_cli_opt(cfg.StrOpt('ghi'))
@@ -813,7 +814,8 @@ 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))
+                                             positional=True,
+                                             required=False))
 
         self.conf(cli_args)
 
@@ -940,19 +942,15 @@ def test_required_positional_opt_undefined(self):
         self.assertRaises(SystemExit, self.conf, ['--help'])
         self.assertIn(' foo\n', sys.stdout.getvalue())
 
-        self.assertRaises(cfg.RequiredOptError, self.conf, [])
+        self.assertRaises(SystemExit, self.conf, [])
 
     def test_optional_positional_opt_defined(self):
         self.conf.register_cli_opt(
             cfg.StrOpt('foo', required=False, positional=True))
 
         self.useFixture(fixtures.MonkeyPatch('sys.stdout', moves.StringIO()))
         self.assertRaises(SystemExit, self.conf, ['--help'])
-        # FIXME(dolphm): Due to bug 1676989, this argument appears as a
-        # required argument in the CLI help. Instead, the following
-        # commented-out code should work:
-        # self.assertIn(' [foo]\n', sys.stdout.getvalue())
-        self.assertIn(' foo\n', sys.stdout.getvalue())
+        self.assertIn(' [foo]\n', sys.stdout.getvalue())
 
         self.conf(['bar'])
 
@@ -965,11 +963,7 @@ def test_optional_positional_opt_undefined(self):
 
         self.useFixture(fixtures.MonkeyPatch('sys.stdout', moves.StringIO()))
         self.assertRaises(SystemExit, self.conf, ['--help'])
-        # FIXME(dolphm): Due to bug 1676989, this argument appears as a
-        # required argument in the CLI help. Instead, the following
-        # commented-out code should work:
-        # self.assertIn(' [foo]\n', sys.stdout.getvalue())
-        self.assertIn(' foo\n', sys.stdout.getvalue())
+        self.assertIn(' [foo]\n', sys.stdout.getvalue())
 
         self.conf([])
 
@@ -982,11 +976,7 @@ def test_optional_positional_hyphenated_opt_defined(self):
 
         self.useFixture(fixtures.MonkeyPatch('sys.stdout', moves.StringIO()))
         self.assertRaises(SystemExit, self.conf, ['--help'])
-        # FIXME(dolphm): Due to bug 1676989, this argument appears as a
-        # required argument in the CLI help. Instead, the following
-        # commented-out code should work:
-        # self.assertIn(' [foo-bar]\n', sys.stdout.getvalue())
-        self.assertIn(' foo-bar\n', sys.stdout.getvalue())
+        self.assertIn(' [foo-bar]\n', sys.stdout.getvalue())
 
         self.conf(['baz'])
         self.assertTrue(hasattr(self.conf, 'foo_bar'))
@@ -1002,11 +992,7 @@ def test_optional_positional_hyphenated_opt_undefined(self):
 
         self.useFixture(fixtures.MonkeyPatch('sys.stdout', moves.StringIO()))
         self.assertRaises(SystemExit, self.conf, ['--help'])
-        # FIXME(dolphm): Due to bug 1676989, this argument appears as a
-        # required argument in the CLI help. Instead, the following
-        # commented-out code should work:
-        # self.assertIn(' [foo-bar]\n', sys.stdout.getvalue())
-        self.assertIn(' foo-bar\n', sys.stdout.getvalue())
+        self.assertIn(' [foo-bar]\n', sys.stdout.getvalue())
 
         self.conf([])
         self.assertTrue(hasattr(self.conf, 'foo_bar'))
@@ -1036,12 +1022,12 @@ def test_required_positional_hyphenated_opt_undefined(self):
         self.assertRaises(SystemExit, self.conf, ['--help'])
         self.assertIn(' foo-bar\n', sys.stdout.getvalue())
 
-        self.assertRaises(cfg.RequiredOptError, self.conf, [])
+        self.assertRaises(SystemExit, self.conf, [])
 
     def test_missing_required_cli_opt(self):
         self.conf.register_cli_opt(
             cfg.StrOpt('foo', required=True, positional=True))
-        self.assertRaises(cfg.RequiredOptError, self.conf, [])
+        self.assertRaises(SystemExit, self.conf, [])
 
     def test_positional_opts_order(self):
         self.conf.register_cli_opts((

releasenotes/notes/positional-arguments-are-required-22ddca72e6f523bf.yaml

@@ -0,0 +1,12 @@
+---
+upgrade:
+  - |
+    Positional options are now required by default, to match argparse's default
+    behavior. To revert this behavior (and maintain optional positional
+    arguments), you need to explicitly specify ``positional=True,
+    required=False`` as part of the options definition.
+fixes:
+  - |
+    On the command line, oslo.config now returns command usage information from
+    argparse (instead of dumping a backtrace) when required arguments are
+    missing.