sphinxext: Separate parsing of group and opts

This makes it easier to reason about going forward as we're going to
make some changes to how this is generated.

Change-Id: Iedd19802e612a647bb2f8f1c133344323e597d0e
(cherry picked from commit 10b03c6b0fa0659af24eb6236688823b399d802e)

2 files changed, 144 insertions, 132 deletions

diff --git a/oslo_config/sphinxext.py b/oslo_config/sphinxext.py
index eb04da5..34a1ef0 100644
--- a/oslo_config/sphinxext.py
+++ b/oslo_config/sphinxext.py
@@ -90,10 +90,106 @@ def _get_choice_text(choice):
     return six.text_type(choice)
 
 
-def _format_group(app, namespace, group_name, group_obj, opt_list):
-    group_name = group_name or 'DEFAULT'
-    app.debug('[oslo.config] %s %s' % (namespace, group_name))
+def _format_opt(opt, group_name):
+    opt_type = _TYPE_DESCRIPTIONS.get(type(opt),
+                                      'unknown type')
+    yield '.. oslo.config:option:: %s' % opt.dest
+    yield ''
+    yield _indent(':Type: %s' % opt_type)
+    for default in generator._format_defaults(opt):
+        if default:
+            default = '``' + default + '``'
+        yield _indent(':Default: %s' % default)
+    if getattr(opt.type, 'min', None) is not None:
+        yield _indent(':Minimum Value: %s' % opt.type.min)
+    if getattr(opt.type, 'max', None) is not None:
+        yield _indent(':Maximum Value: %s' % opt.type.max)
+    if getattr(opt.type, 'choices', None):
+        choices_text = ', '.join([_get_choice_text(choice)
+                                  for choice in opt.type.choices])
+        yield _indent(':Valid Values: %s' % choices_text)
+    try:
+        if opt.mutable:
+            yield _indent(
+                ':Mutable: This option can be changed without restarting.'
+            )
+    except AttributeError as err:
+        # NOTE(dhellmann): keystoneauth defines its own Opt class,
+        # and neutron (at least) returns instances of those
+        # classes instead of oslo_config Opt instances. The new
+        # mutable attribute is the first property where the API
+        # isn't supported in the external class, so we can use
+        # this failure to emit a warning. See
+        # https://bugs.launchpad.net/keystoneauth/+bug/1548433 for
+        # more details.
+        import warnings
+        if not isinstance(cfg.Opt, opt):
+            warnings.warn(
+                'Incompatible option class for %s (%r): %s' %
+                (opt.dest, opt.__class__, err),
+            )
+        else:
+            warnings.warn('Failed to fully format sample for %s: %s' %
+                          (opt.dest, err))
+    if opt.advanced:
+        yield _indent(
+            ':Advanced Option: Intended for advanced users and not used')
+        yield _indent(
+            'by the majority of users, and might have a significant', 6)
+        yield _indent(
+            'effect on stability and/or performance.', 6)
+
+    try:
+        help_text = opt.help % {'default': 'the value above'}
+    except (TypeError, KeyError, ValueError):
+        # There is no mention of the default in the help string,
+        # the string had some unknown key, or the string contained
+        # invalid formatting characters
+        help_text = opt.help
+    if help_text:
+        yield ''
+        yield _indent(help_text.strip())
+
+    # We don't bother outputting this if not using new-style choices with
+    # inline descriptions
+    if getattr(opt.type, 'choices', None) and not all(
+            x is None for x in opt.type.choices.values()):
+        yield ''
+        yield _indent('.. rubric:: Possible values')
+        for choice in opt.type.choices:
+            yield ''
+            yield _indent(_get_choice_text(choice))
+            yield _indent(_indent(
+                opt.type.choices[choice] or '<No description provided>'))
+
+    if opt.deprecated_opts:
+        yield ''
+        for line in _list_table(
+                ['Group', 'Name'],
+                ((d.group or group_name,
+                  d.name or opt.dest or 'UNSET')
+                 for d in opt.deprecated_opts),
+                title='Deprecated Variations'):
+            yield _indent(line)
+
+    if opt.deprecated_for_removal:
+        yield ''
+        yield _indent('.. warning::')
+        if opt.deprecated_since:
+            yield _indent('   This option is deprecated for removal '
+                          'since %s.' % opt.deprecated_since)
+        else:
+            yield _indent('   This option is deprecated for removal.')
+        yield _indent('   Its value may be silently ignored ')
+        yield _indent('   in the future.')
+        if opt.deprecated_reason:
+            yield ''
+            yield _indent('   :Reason: ' + opt.deprecated_reason)
+
+    yield ''
 
+
+def _format_group(namespace, group_name, group_obj):
     yield '.. oslo.config:group:: %s' % group_name
     if namespace:
         yield '   :namespace: %s' % namespace
@@ -103,103 +199,17 @@ def _format_group(app, namespace, group_name, group_obj, opt_list):
         yield _indent(group_obj.help.rstrip())
         yield ''
 
-    for opt in opt_list:
-        opt_type = _TYPE_DESCRIPTIONS.get(type(opt),
-                                          'unknown type')
-        yield '.. oslo.config:option:: %s' % opt.dest
-        yield ''
-        yield _indent(':Type: %s' % opt_type)
-        for default in generator._format_defaults(opt):
-            if default:
-                default = '``' + default + '``'
-            yield _indent(':Default: %s' % default)
-        if getattr(opt.type, 'min', None) is not None:
-            yield _indent(':Minimum Value: %s' % opt.type.min)
-        if getattr(opt.type, 'max', None) is not None:
-            yield _indent(':Maximum Value: %s' % opt.type.max)
-        if getattr(opt.type, 'choices', None):
-            choices_text = ', '.join([_get_choice_text(choice)
-                                      for choice in opt.type.choices])
-            yield _indent(':Valid Values: %s' % choices_text)
-        try:
-            if opt.mutable:
-                yield _indent(
-                    ':Mutable: This option can be changed without restarting.'
-                )
-        except AttributeError as err:
-            # NOTE(dhellmann): keystoneauth defines its own Opt class,
-            # and neutron (at least) returns instances of those
-            # classes instead of oslo_config Opt instances. The new
-            # mutable attribute is the first property where the API
-            # isn't supported in the external class, so we can use
-            # this failure to emit a warning. See
-            # https://bugs.launchpad.net/keystoneauth/+bug/1548433 for
-            # more details.
-            import warnings
-            if not isinstance(cfg.Opt, opt):
-                warnings.warn(
-                    'Incompatible option class for %s (%r): %s' %
-                    (opt.dest, opt.__class__, err),
-                )
-            else:
-                warnings.warn('Failed to fully format sample for %s: %s' %
-                              (opt.dest, err))
-        if opt.advanced:
-            yield _indent(
-                ':Advanced Option: Intended for advanced users and not used')
-            yield _indent(
-                'by the majority of users, and might have a significant', 6)
-            yield _indent(
-                'effect on stability and/or performance.', 6)
-
-        try:
-            help_text = opt.help % {'default': 'the value above'}
-        except (TypeError, KeyError, ValueError):
-            # There is no mention of the default in the help string,
-            # the string had some unknown key, or the string contained
-            # invalid formatting characters
-            help_text = opt.help
-        if help_text:
-            yield ''
-            yield _indent(help_text)
 
-        # We don't bother outputting this if not using new-style choices with
-        # inline descriptions
-        if getattr(opt.type, 'choices', None) and not all(
-                x is None for x in opt.type.choices.values()):
-            yield ''
-            yield _indent('.. rubric:: Possible values')
-            for choice in opt.type.choices:
-                yield ''
-                yield _indent(_get_choice_text(choice))
-                yield _indent(_indent(
-                    opt.type.choices[choice] or '<No description provided>'))
-
-        if opt.deprecated_opts:
-            yield ''
-            for line in _list_table(
-                    ['Group', 'Name'],
-                    ((d.group or group_name,
-                      d.name or opt.dest or 'UNSET')
-                     for d in opt.deprecated_opts),
-                    title='Deprecated Variations'):
-                yield _indent(line)
-
-        if opt.deprecated_for_removal:
-            yield ''
-            yield _indent('.. warning::')
-            if opt.deprecated_since:
-                yield _indent('   This option is deprecated for removal '
-                              'since %s.' % opt.deprecated_since)
-            else:
-                yield _indent('   This option is deprecated for removal.')
-            yield _indent('   Its value may be silently ignored ')
-            yield _indent('   in the future.')
-            if opt.deprecated_reason:
-                yield ''
-                yield _indent('   :Reason: ' + opt.deprecated_reason)
+def _format_group_opts(app, namespace, group_name, group_obj, opt_list):
+    group_name = group_name or 'DEFAULT'
+    app.debug('[oslo.config] %s %s' % (namespace, group_name))
 
-        yield ''
+    for line in _format_group(namespace, group_name, group_obj):
+        yield line
+
+    for opt in opt_list:
+        for line in _format_opt(opt, group_name):
+            yield line
 
 
 def _format_option_help(app, namespaces, split_namespaces):
@@ -220,7 +230,7 @@ def _format_option_help(app, namespaces, split_namespaces):
                     group = None
                 if group_name is None:
                     group_name = 'DEFAULT'
-                lines = _format_group(
+                lines = _format_group_opts(
                     app=app,
                     namespace=namespace,
                     group_name=group_name,
@@ -247,7 +257,7 @@ def _format_option_help(app, namespaces, split_namespaces):
                 group_objs.setdefault(group_name, group)
                 by_section.setdefault(group_name, []).extend(group_opts)
         for group_name, group_opts in sorted(by_section.items()):
-            lines = _format_group(
+            lines = _format_group_opts(
                 app=app,
                 namespace=None,
                 group_name=group_name,
diff --git a/oslo_config/tests/test_sphinxext.py b/oslo_config/tests/test_sphinxext.py
index b1f4410..5a89e8a 100644
--- a/oslo_config/tests/test_sphinxext.py
+++ b/oslo_config/tests/test_sphinxext.py
@@ -23,7 +23,7 @@ class FormatGroupTest(base.BaseTestCase):
 
     def test_none_in_default(self):
         # option with None group placed in DEFAULT
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -45,7 +45,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_with_default_value(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -68,7 +68,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_with_min(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -89,7 +89,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_with_min_0(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -110,7 +110,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_with_max(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -131,7 +131,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_with_max_0(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -152,7 +152,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_with_choices(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -173,7 +173,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_with_choices_with_descriptions(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -218,7 +218,7 @@ class FormatGroupTest(base.BaseTestCase):
 
     def test_group_obj_without_help(self):
         # option with None group placed in DEFAULT
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name='group',
@@ -236,7 +236,7 @@ class FormatGroupTest(base.BaseTestCase):
 
     def test_group_obj_with_help(self):
         # option with None group placed in DEFAULT
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name='group',
@@ -255,7 +255,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_deprecated_opts_without_deprecated_group(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -284,7 +284,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_deprecated_opts_with_deprecated_group(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -314,7 +314,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_deprecated_for_removal(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -332,7 +332,7 @@ class FormatGroupTest(base.BaseTestCase):
         self.assertIn('since 13.0', results)
 
     def test_mutable(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -353,7 +353,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_not_mutable(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -373,7 +373,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_advanced(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -396,7 +396,7 @@ class FormatGroupTest(base.BaseTestCase):
         ''').lstrip(), results)
 
     def test_not_advanced(self):
-        results = '\n'.join(list(sphinxext._format_group(
+        results = '\n'.join(list(sphinxext._format_group_opts(
             app=mock.Mock(),
             namespace=None,
             group_name=None,
@@ -419,8 +419,8 @@ class FormatGroupTest(base.BaseTestCase):
 class FormatOptionHelpTest(base.BaseTestCase):
 
     @mock.patch('oslo_config.generator._list_opts')
-    @mock.patch('oslo_config.sphinxext._format_group')
-    def test_split_namespaces(self, _format_group, _list_opts):
+    @mock.patch('oslo_config.sphinxext._format_group_opts')
+    def test_split_namespaces(self, _format_group_opts, _list_opts):
         _list_opts.return_value = [
             ('namespace1', [(None, ['opt1'])]),
             ('namespace2', [(None, ['opt2'])]),
@@ -429,14 +429,14 @@ class FormatOptionHelpTest(base.BaseTestCase):
             app=None,
             namespaces=['namespace1', 'namespace2'],
             split_namespaces=True))
-        _format_group.assert_any_call(
+        _format_group_opts.assert_any_call(
             app=None,
             namespace='namespace1',
             group_name='DEFAULT',
             group_obj=None,
             opt_list=['opt1'],
         )
-        _format_group.assert_any_call(
+        _format_group_opts.assert_any_call(
             app=None,
             namespace='namespace2',
             group_name='DEFAULT',
@@ -445,8 +445,8 @@ class FormatOptionHelpTest(base.BaseTestCase):
         )
 
     @mock.patch('oslo_config.generator._list_opts')
-    @mock.patch('oslo_config.sphinxext._format_group')
-    def test_dont_split_namespaces(self, _format_group, _list_opts):
+    @mock.patch('oslo_config.sphinxext._format_group_opts')
+    def test_dont_split_namespaces(self, _format_group_opts, _list_opts):
         _list_opts.return_value = [
             ('namespace1', [(None, ['opt1'])]),
             ('namespace2', [(None, ['opt2'])]),
@@ -455,7 +455,7 @@ class FormatOptionHelpTest(base.BaseTestCase):
             app=None,
             namespaces=['namespace1', 'namespace2'],
             split_namespaces=False))
-        _format_group.assert_called_once_with(
+        _format_group_opts.assert_called_once_with(
             app=None,
             namespace=None,
             group_name='DEFAULT',
@@ -464,8 +464,9 @@ class FormatOptionHelpTest(base.BaseTestCase):
         )
 
     @mock.patch('oslo_config.generator._list_opts')
-    @mock.patch('oslo_config.sphinxext._format_group')
-    def test_dont_split_namespaces_with_group(self, _format_group, _list_opts):
+    @mock.patch('oslo_config.sphinxext._format_group_opts')
+    def test_dont_split_namespaces_with_group(self, _format_group_opts,
+                                              _list_opts):
         grp_obj = cfg.OptGroup('grp1')
         _list_opts.return_value = [
             ('namespace1', [(grp_obj, ['opt1'])]),
@@ -475,7 +476,7 @@ class FormatOptionHelpTest(base.BaseTestCase):
             app=None,
             namespaces=['namespace1', 'namespace2'],
             split_namespaces=False))
-        _format_group.assert_any_call(
+        _format_group_opts.assert_any_call(
             app=None,
             namespace=None,
             group_name='grp1',
@@ -484,8 +485,9 @@ class FormatOptionHelpTest(base.BaseTestCase):
         )
 
     @mock.patch('oslo_config.generator._list_opts')
-    @mock.patch('oslo_config.sphinxext._format_group')
-    def test_split_namespaces_with_group(self, _format_group, _list_opts):
+    @mock.patch('oslo_config.sphinxext._format_group_opts')
+    def test_split_namespaces_with_group(self, _format_group_opts,
+                                         _list_opts):
         grp_obj = cfg.OptGroup('grp1')
         _list_opts.return_value = [
             ('namespace1', [(grp_obj, ['opt1'])]),
@@ -495,15 +497,15 @@ class FormatOptionHelpTest(base.BaseTestCase):
             app=None,
             namespaces=['namespace1', 'namespace2'],
             split_namespaces=True))
-        print(_format_group.call_args_list)
-        _format_group.assert_any_call(
+        print(_format_group_opts.call_args_list)
+        _format_group_opts.assert_any_call(
             app=None,
             namespace='namespace1',
             group_name='grp1',
             group_obj=grp_obj,
             opt_list=['opt1'],
         )
-        _format_group.assert_any_call(
+        _format_group_opts.assert_any_call(
             app=None,
             namespace='namespace2',
             group_name='grp1',