From 18d1617caa5a7380e2298b1e647ef110c9d574be Mon Sep 17 00:00:00 2001 From: Dolph Mathews Date: Thu, 30 Mar 2017 18:24:31 +0000 Subject: [PATCH] Assume positional arguments are required The 'positional' keyword specifically applies to oslo.config's argparse support. Unlike oslo.config, argparse assumes that all positional arguments are required by default, and you have to explicitly tell it that a positional argument is optional if you'd like to opt into that behavior. This patch adopts that same behavior for oslo.config. When you define an option to be non-positional (oslo.config's default, designed for config files), then oslo.config makes that option optional: However, when you define an option to be positional, oslo.config assumes that the option is primarily going to be used on the CLI and thus sets it as required, by default. This change in behavior has the side effect of allowing argparse to enforce required arguments on the CLI *while* parsing arguments, instead of depending on oslo.config to detect the condition *after* argparse has been allowed to parse "invalid" arguments. argparse correctly raises a SystemExit in this case, and prints the actual command usage and a "hey, you forgot this required argument", instead of allowing oslo.config to dump a backtrace to the CLI with a context-less error message ("context-less" in that no useful CLI usage information is dumped along with the crash to help you correct the condition). Change-Id: Ifdc6918444fe72f7e1649483c237cce64b4c72d8 Partial-Bug: 1676989 --- doc/source/reference/command-line.rst | 4 +++ oslo_config/cfg.py | 9 +++-- oslo_config/tests/test_cfg.py | 36 ++++++------------- ...guments-are-required-22ddca72e6f523bf.yaml | 12 +++++++ 4 files changed, 34 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/positional-arguments-are-required-22ddca72e6f523bf.yaml diff --git a/doc/source/reference/command-line.rst b/doc/source/reference/command-line.rst index 169b4ba5..5a4cbbb4 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 5dcb3f17..d36245f9 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 a7d77673..c9f04146 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 00000000..6e7914c7 --- /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.