From e447675ef8a24665bfe3e5c22389f1c73b79923f Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Tue, 18 Nov 2014 05:14:32 +0300 Subject: [PATCH] Fix wrong order of positional args in cli Positional arguments were incorrectly parsed because instead of original order there was an alphabetical. This patch brings a positional field in Opt class and then sorts all arguments by position-name. Change-Id: Ia22654442b895c3f62834428f0ec6c3e8af22ebb Closes-Bug: 1392428 --- oslo/config/cfg.py | 37 ++++++++++++++++++++++++------ tests/test_cfg.py | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/oslo/config/cfg.py b/oslo/config/cfg.py index 63aecff..a6fe1a1 100644 --- a/oslo/config/cfg.py +++ b/oslo/config/cfg.py @@ -1570,8 +1570,17 @@ class _CachedArgumentParser(argparse.ArgumentParser): self._args_cache[container] = values def initialize_parser_arguments(self): + # NOTE(mfedosin): The code below looks a little bit weird, but + # it's done because we need to sort only optional opts and do + # not touch positional. For the reason optional opts go first in + # the values we only need to find an index of the first positional + # option and then sort the values slice. for container, values in six.iteritems(self._args_cache): - values.sort(key=lambda x: x['args']) + index = 0 + for index, argument in enumerate(values): + if not argument['args'][0].startswith('-'): + break + values[:index] = sorted(values[:index], key=lambda x: x['args']) for argument in values: try: container.add_argument(*argument['args'], @@ -1613,6 +1622,7 @@ class ConfigOpts(collections.Mapping): self._namespace = None self.__cache = {} self._config_opts = [] + self._cli_opts = collections.deque() self._validate_default_values = False def _pre_setup(self, project, prog, version, usage, default_config_files): @@ -1781,6 +1791,14 @@ class ConfigOpts(collections.Mapping): for group in self._groups.values(): group._clear() + def _add_cli_opt(self, opt, group): + if {'opt': opt, 'group': group} in self._cli_opts: + return + if opt.positional: + self._cli_opts.append({'opt': opt, 'group': group}) + else: + self._cli_opts.appendleft({'opt': opt, 'group': group}) + @__clear_cache def register_opt(self, opt, group=None, cli=False): """Register an option schema. @@ -1797,8 +1815,13 @@ class ConfigOpts(collections.Mapping): """ if group is not None: group = self._get_group(group, autocreate=True) + if cli: + self._add_cli_opt(opt, group) return group._register_opt(opt, cli) + if cli: + self._add_cli_opt(opt, None) + if _is_opt_registered(self._opts, opt): return False @@ -1860,6 +1883,9 @@ class ConfigOpts(collections.Mapping): if self._args is not None: raise ArgsAlreadyParsedError("reset before unregistering options") + if {'opt': opt, 'group': group} in self._cli_opts: + self._cli_opts.remove({'opt': opt, 'group': group}) + if group is not None: self._get_group(group)._unregister_opt(opt) elif opt.dest in self._opts: @@ -1975,9 +2001,8 @@ class ConfigOpts(collections.Mapping): def _all_cli_opts(self): """A generator function for iterating CLI opts.""" - for info, group in self._all_opt_infos(): - if info['cli']: - yield info['opt'], group + for item in self._cli_opts: + yield item['opt'], item['group'] def _unset_defaults_and_overrides(self): """Unset any default or override on all options.""" @@ -2267,9 +2292,7 @@ class ConfigOpts(collections.Mapping): """ self._args = args - - for opt, group in sorted(self._all_cli_opts(), - key=lambda x: x[0].name): + for opt, group in self._all_cli_opts(): opt._add_to_cli(self._oparser, group) return self._parse_config_files() diff --git a/tests/test_cfg.py b/tests/test_cfg.py index 01823b1..0943348 100644 --- a/tests/test_cfg.py +++ b/tests/test_cfg.py @@ -622,6 +622,62 @@ class PositionalTestCase(BaseTestCase): cfg.StrOpt('foo', required=True, positional=True)) self.assertRaises(cfg.RequiredOptError, self.conf, []) + def test_positional_opts_order(self): + self.conf.register_cli_opts(( + cfg.StrOpt('command', positional=True), + cfg.StrOpt('arg1', positional=True), + cfg.StrOpt('arg2', positional=True)) + ) + + self.conf(['command', 'arg1', 'arg2']) + + self.assertEqual('command', self.conf.command) + self.assertEqual('arg1', self.conf.arg1) + self.assertEqual('arg2', self.conf.arg2) + + def test_positional_opt_order(self): + self.conf.register_cli_opt( + cfg.StrOpt('command', positional=True)) + self.conf.register_cli_opt( + cfg.StrOpt('arg1', positional=True)) + self.conf.register_cli_opt( + cfg.StrOpt('arg2', positional=True)) + + self.conf(['command', 'arg1', 'arg2']) + + self.assertEqual('command', self.conf.command) + self.assertEqual('arg1', self.conf.arg1) + self.assertEqual('arg2', self.conf.arg2) + + def test_positional_opt_unregister(self): + command = cfg.StrOpt('command', positional=True) + arg1 = cfg.StrOpt('arg1', positional=True) + arg2 = cfg.StrOpt('arg2', positional=True) + self.conf.register_cli_opt(command) + self.conf.register_cli_opt(arg1) + self.conf.register_cli_opt(arg2) + + self.conf(['command', 'arg1', 'arg2']) + + self.assertEqual('command', self.conf.command) + self.assertEqual('arg1', self.conf.arg1) + self.assertEqual('arg2', self.conf.arg2) + + self.conf.reset() + + self.conf.unregister_opt(arg1) + self.conf.unregister_opt(arg2) + + arg0 = cfg.StrOpt('arg0', positional=True) + self.conf.register_cli_opt(arg0) + self.conf.register_cli_opt(arg1) + + self.conf(['command', 'arg0', 'arg1']) + + self.assertEqual('command', self.conf.command) + self.assertEqual('arg0', self.conf.arg0) + self.assertEqual('arg1', self.conf.arg1) + class ConfigFileOptsTestCase(BaseTestCase):