Merge "Assume positional arguments are required"

4 files changed, 34 insertions, 27 deletions

diff --git a/doc/source/reference/command-line.rst b/doc/source/reference/command-line.rst
index 169b4ba..5a4cbbb 100644
--- a/doc/source/reference/command-line.rst
+++ b/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
 -----------
 
diff --git a/oslo_config/cfg.py b/oslo_config/cfg.py
index 5dcb3f1..d36245f 100644
--- a/oslo_config/cfg.py
+++ b/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 @@ class Opt(object):
             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 @@ class Opt(object):
             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,
diff --git a/oslo_config/tests/test_cfg.py b/oslo_config/tests/test_cfg.py
index a7d7767..c9f0414 100644
--- a/oslo_config/tests/test_cfg.py
+++ b/oslo_config/tests/test_cfg.py
@@ -234,7 +234,8 @@ class HelpTestCase(BaseTestCase):
 
     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,7 +942,7 @@ class PositionalTestCase(BaseTestCase):
         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(
@@ -948,11 +950,7 @@ class PositionalTestCase(BaseTestCase):
 
         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 @@ class PositionalTestCase(BaseTestCase):
 
         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 @@ class PositionalTestCase(BaseTestCase):
 
         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 @@ class PositionalTestCase(BaseTestCase):
 
         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 @@ class PositionalTestCase(BaseTestCase):
         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((
diff --git a/releasenotes/notes/positional-arguments-are-required-22ddca72e6f523bf.yaml b/releasenotes/notes/positional-arguments-are-required-22ddca72e6f523bf.yaml
new file mode 100644
index 0000000..6e7914c
--- /dev/null
+++ b/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.