Merge "Provide descriptions for choices"

8 files changed, 227 insertions, 73 deletions

diff --git a/oslo_config/cfg.py b/oslo_config/cfg.py
index 1c003ea..5bd81bf 100644
--- a/oslo_config/cfg.py
+++ b/oslo_config/cfg.py
@@ -1225,7 +1225,8 @@ class StrOpt(Opt):
     Option with ``type`` :class:`oslo_config.types.String`
 
     :param name: the option's name
-    :param choices: Optional sequence of valid values.
+    :param choices: Optional sequence of either valid values or tuples of valid
+        values with descriptions.
     :param quotes: If True and string is enclosed with single or double
                    quotes, will strip those quotes.
     :param regex: Optional regular expression (string or compiled
@@ -1248,6 +1249,10 @@ class StrOpt(Opt):
 
     .. versionchanged:: 2.7
        Added *max_length* parameter
+
+    .. versionchanged:: 5.2
+       The *choices* parameter will now accept a sequence of tuples, where each
+       tuple is of form (*choice*, *description*)
     """
 
     def __init__(self, name, choices=None, quotes=None,
@@ -1275,10 +1280,11 @@ class StrOpt(Opt):
         if getattr(self.type, 'choices', None):
             choices_text = ', '.join([self._get_choice_text(choice)
                                       for choice in self.type.choices])
-            if kwargs['help'] is not None:
-                kwargs['help'] += (' Allowed values: %s\n' % choices_text)
-            else:
-                kwargs['help'] = (' Allowed values: %s\n' % choices_text)
+            if kwargs['help'] is None:
+                kwargs['help'] = ''
+
+            kwargs['help'].rstrip('\n')
+            kwargs['help'] += '\n Allowed values: %s\n' % choices_text
 
         return kwargs
 
@@ -1448,7 +1454,8 @@ class PortOpt(Opt):
     :param name: the option's name
     :param min: minimum value the port can take
     :param max: maximum value the port can take
-    :param choices: Optional sequence of valid values.
+    :param choices: Optional sequence of either valid values or tuples of valid
+        values with descriptions.
     :param \*\*kwargs: arbitrary keyword arguments passed to :class:`Opt`
 
     .. versionadded:: 2.6
@@ -1458,6 +1465,9 @@ class PortOpt(Opt):
        Allow port number with 0.
     .. versionchanged:: 3.16
        Added *min* and *max* parameters.
+    .. versionchanged:: 5.2
+       The *choices* parameter will now accept a sequence of tuples, where each
+       tuple is of form (*choice*, *description*)
     """
 
     def __init__(self, name, min=None, max=None, choices=None, **kwargs):
diff --git a/oslo_config/generator.py b/oslo_config/generator.py
index 7f622be..7b6e5ad 100644
--- a/oslo_config/generator.py
+++ b/oslo_config/generator.py
@@ -66,14 +66,18 @@ _generator_opts = [
         'longer help text for Sphinx documents.'),
     cfg.StrOpt(
         'format',
-        help='Desired format for the output. "ini" is the only one which can '
-             'be used directly with oslo.config. "json" and "yaml" are '
-             'intended for third-party tools that want to write config files '
-             'based on the sample config data. "rst" can be used to dump '
-             'the text given to sphinx when building documentation using '
-             'the sphinx extension, for debugging.',
+        help='Desired format for the output.',
         default='ini',
-        choices=['ini', 'json', 'yaml', 'rst'],
+        choices=[
+            ('ini', 'The only format that can be used directly with '
+             'oslo.config.'),
+            ('json', 'Intended for third-party tools that want to write '
+             'config files based on the sample config data.'),
+            ('yaml', 'Same as json'),
+            ('rst', 'Can be used to dump the text given to Sphinx when '
+             'building documentation using the Sphinx extension. '
+             'Useful for debugging,')
+        ],
         dest='format_'),
 ]
 
@@ -257,9 +261,12 @@ class _OptFormatter(object):
             lines.append('# Maximum value: %d\n' % opt.type.max)
 
         if getattr(opt.type, 'choices', None):
-            choices_text = ', '.join([self._get_choice_text(choice)
-                                      for choice in opt.type.choices])
-            lines.append('# Allowed values: %s\n' % choices_text)
+            lines.append('# Possible values:\n')
+            for choice in opt.type.choices:
+                help_text = '%s - %s' % (
+                    self._get_choice_text(choice),
+                    opt.type.choices[choice] or '<No description provided>')
+                lines.extend(self._format_help(help_text))
 
         try:
             if opt.mutable:
@@ -581,9 +588,14 @@ def _build_entry(opt, group, namespace, conf):
     entry = {key: value for key, value in opt.__dict__.items()
              if not key.startswith('_')}
     entry['namespace'] = namespace
-    # In some types, choices is explicitly set to None.  Force it to [] so it
-    # is always an iterable type.
-    entry['choices'] = getattr(entry['type'], 'choices', []) or []
+    # Where present, we store choices as an OrderedDict. The default repr for
+    # this is not very machine readable, thus, it is switched to a list of
+    # tuples here. In addition, in some types, choices is explicitly set to
+    # None. Force these cases to [] so it is always an iterable type.
+    if getattr(entry['type'], 'choices', None):
+        entry['choices'] = list(entry['type'].choices.items())
+    else:
+        entry['choices'] = []
     entry['min'] = getattr(entry['type'], 'min', None)
     entry['max'] = getattr(entry['type'], 'max', None)
     entry['type'] = _format_type_name(entry['type'])
diff --git a/oslo_config/sphinxext.py b/oslo_config/sphinxext.py
index 580d605..bfd359b 100644
--- a/oslo_config/sphinxext.py
+++ b/oslo_config/sphinxext.py
@@ -165,6 +165,18 @@ def _format_group(app, namespace, group_name, group_obj, opt_list):
             yield _indent(help_text)
             yield ''
 
+        # 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 _indent('.. rubric:: Possible values')
+            yield ''
+            for choice in opt.type.choices:
+                yield _indent(_get_choice_text(choice))
+                yield _indent(_indent(
+                    opt.type.choices[choice] or '<No description provided>'))
+                yield ''
+
         if opt.deprecated_opts:
             for line in _list_table(
                     ['Group', 'Name'],
diff --git a/oslo_config/tests/test_generator.py b/oslo_config/tests/test_generator.py
index 4c92eb5..e01bbab 100644
--- a/oslo_config/tests/test_generator.py
+++ b/oslo_config/tests/test_generator.py
@@ -431,7 +431,12 @@ class GeneratorTestCase(base.BaseTestCase):
 #
 
 # a string with choices (string value)
-# Allowed values: <None>, '', a, b, c
+# Possible values:
+# <None> - <No description provided>
+# '' - <No description provided>
+# a - <No description provided>
+# b - <No description provided>
+# c - <No description provided>
 #choices_opt = a
 ''')),
         ('deprecated opt without deprecated group',
@@ -1219,7 +1224,13 @@ class MachineReadableGeneratorTestCase(base.BaseTestCase):
                                 'help': '',
                                 'standard_opts': ['choices_opt'],
                                 'opts': [{'advanced': False,
-                                          'choices': (None, '', 'a', 'b', 'c'),
+                                          'choices': [
+                                              (None, None),
+                                              ('', None),
+                                              ('a', None),
+                                              ('b', None),
+                                              ('c', None)
+                                          ],
                                           'default': 'a',
                                           'deprecated_for_removal': False,
                                           'deprecated_opts': [],
diff --git a/oslo_config/tests/test_sphinxext.py b/oslo_config/tests/test_sphinxext.py
index 416dabe..aee69e1 100644
--- a/oslo_config/tests/test_sphinxext.py
+++ b/oslo_config/tests/test_sphinxext.py
@@ -179,6 +179,51 @@ class FormatGroupTest(base.BaseTestCase):
 
         ''').lstrip(), results)
 
+    def test_with_choices_with_descriptions(self):
+        results = '\n'.join(list(sphinxext._format_group(
+            app=mock.Mock(),
+            namespace=None,
+            group_name=None,
+            group_obj=None,
+            opt_list=[
+                cfg.StrOpt(
+                    'opt_name',
+                    choices=[
+                        ('a', 'a is the best'),
+                        ('b', 'Actually, may-b I am better'),
+                        ('c', 'c, I am clearly the greatest'),
+                        (None, 'I am having none of this'),
+                        ('', '')]),
+            ],
+        )))
+        self.assertEqual(textwrap.dedent('''
+        .. oslo.config:group:: DEFAULT
+
+        .. oslo.config:option:: opt_name
+
+          :Type: string
+          :Default: ``<None>``
+          :Valid Values: a, b, c, <None>, ''
+
+          .. rubric:: Possible values
+
+          a
+            a is the best
+
+          b
+            Actually, may-b I am better
+
+          c
+            c, I am clearly the greatest
+
+          <None>
+            I am having none of this
+
+          ''
+            <No description provided>
+
+        ''').lstrip(), results)
+
     def test_group_obj_without_help(self):
         # option with None group placed in DEFAULT
         results = '\n'.join(list(sphinxext._format_group(
diff --git a/oslo_config/tests/test_types.py b/oslo_config/tests/test_types.py
index 1e357bd..dc8c501 100644
--- a/oslo_config/tests/test_types.py
+++ b/oslo_config/tests/test_types.py
@@ -67,6 +67,11 @@ class StringTypeTests(TypeTestHelper, unittest.TestCase):
         self.type_instance = types.String(choices=('foo', 'bar'))
         self.assertConvertedValue('foo', 'foo')
 
+    def test_listed_value_dict(self):
+        self.type_instance = types.String(choices=[
+            ('foo', 'ab'), ('bar', 'xy')])
+        self.assertConvertedValue('foo', 'foo')
+
     def test_unlisted_value(self):
         self.type_instance = types.String(choices=['foo', 'bar'])
         self.assertInvalid('baz')
@@ -98,7 +103,11 @@ class StringTypeTests(TypeTestHelper, unittest.TestCase):
 
     def test_repr_with_choices_tuple(self):
         t = types.String(choices=('foo', 'bar'))
-        self.assertEqual('String(choices=(\'foo\', \'bar\'))', repr(t))
+        self.assertEqual('String(choices=[\'foo\', \'bar\'])', repr(t))
+
+    def test_repr_with_choices_dict(self):
+        t = types.String(choices=[('foo', 'ab'), ('bar', 'xy')])
+        self.assertEqual('String(choices=[\'foo\', \'bar\'])', repr(t))
 
     def test_equal(self):
         self.assertTrue(types.String() == types.String())
@@ -108,9 +117,8 @@ class StringTypeTests(TypeTestHelper, unittest.TestCase):
         t2 = types.String(choices=['foo', 'bar'])
         t3 = types.String(choices=('foo', 'bar'))
         t4 = types.String(choices=['bar', 'foo'])
-        self.assertTrue(t1 == t2)
-        self.assertTrue(t1 == t3)
-        self.assertTrue(t1 == t4)
+        t5 = types.String(choices=[('foo', 'ab'), ('bar', 'xy')])
+        self.assertTrue(t1 == t2 == t3 == t4 == t5)
 
     def test_not_equal_with_different_choices(self):
         t1 = types.String(choices=['foo', 'bar'])
@@ -282,7 +290,11 @@ class IntegerTypeTests(TypeTestHelper, unittest.TestCase):
 
     def test_repr_with_choices_tuple(self):
         t = types.Integer(choices=(80, 457))
-        self.assertEqual('Integer(choices=(80, 457))', repr(t))
+        self.assertEqual('Integer(choices=[80, 457])', repr(t))
+
+    def test_repr_with_choices_dict(self):
+        t = types.Integer(choices=[(80, 'ab'), (457, 'xy')])
+        self.assertEqual('Integer(choices=[80, 457])', repr(t))
 
     def test_equal(self):
         self.assertTrue(types.Integer() == types.Integer())
@@ -302,8 +314,8 @@ class IntegerTypeTests(TypeTestHelper, unittest.TestCase):
         t1 = types.Integer(choices=[80, 457])
         t2 = types.Integer(choices=[457, 80])
         t3 = types.Integer(choices=(457, 80))
-        self.assertTrue(t1 == t2)
-        self.assertTrue(t1 == t3)
+        t4 = types.Integer(choices=[(80, 'ab'), (457, 'xy')])
+        self.assertTrue(t1 == t2 == t3 == t4)
 
     def test_not_equal(self):
         self.assertFalse(types.Integer(min=123) == types.Integer(min=456))
@@ -369,21 +381,24 @@ class IntegerTypeTests(TypeTestHelper, unittest.TestCase):
         self.assertRaises(ValueError, t, 201)
         self.assertRaises(ValueError, t, -457)
 
-    def test_with_choices_list(self):
-        t = types.Integer(choices=[80, 457])
+    def _test_with_choices(self, t):
         self.assertRaises(ValueError, t, 1)
         self.assertRaises(ValueError, t, 200)
         self.assertRaises(ValueError, t, -457)
         t(80)
         t(457)
 
+    def test_with_choices_list(self):
+        t = types.Integer(choices=[80, 457])
+        self._test_with_choices(t)
+
     def test_with_choices_tuple(self):
         t = types.Integer(choices=(80, 457))
-        self.assertRaises(ValueError, t, 1)
-        self.assertRaises(ValueError, t, 200)
-        self.assertRaises(ValueError, t, -457)
-        t(80)
-        t(457)
+        self._test_with_choices(t)
+
+    def test_with_choices_dict(self):
+        t = types.Integer(choices=[(80, 'ab'), (457, 'xy')])
+        self._test_with_choices(t)
 
 
 class FloatTypeTests(TypeTestHelper, unittest.TestCase):
@@ -865,16 +880,29 @@ class PortTypeTests(TypeTestHelper, unittest.TestCase):
 
     def test_repr_with_choices_tuple(self):
         t = types.Port(choices=(80, 457))
-        self.assertEqual('Port(choices=(80, 457))', repr(t))
+        self.assertEqual('Port(choices=[80, 457])', repr(t))
 
-    def test_choices(self):
-        t = types.Port(choices=[80, 457])
+    def _test_with_choices(self, t):
         self.assertRaises(ValueError, t, 1)
         self.assertRaises(ValueError, t, 200)
+        self.assertRaises(ValueError, t, -457)
         t(80)
         t(457)
 
+    def test_with_choices_list(self):
+        t = types.Port(choices=[80, 457])
+        self._test_with_choices(t)
+
+    def test_with_choices_tuple(self):
+        t = types.Port(choices=(80, 457))
+        self._test_with_choices(t)
+
+    def test_with_choices_dict(self):
+        t = types.Port(choices=[(80, 'ab'), (457, 'xy')])
+        self._test_with_choices(t)
+
     def test_invalid_choices(self):
+        """Check for choices that are specifically invalid for ports."""
         self.assertRaises(ValueError, types.Port, choices=[-1, 457])
         self.assertRaises(ValueError, types.Port, choices=[1, 2, 3, 65536])
 
@@ -896,8 +924,8 @@ class PortTypeTests(TypeTestHelper, unittest.TestCase):
         t1 = types.Port(choices=[80, 457])
         t2 = types.Port(choices=[457, 80])
         t3 = types.Port(choices=(457, 80))
-        self.assertTrue(t1 == t2)
-        self.assertTrue(t1 == t3)
+        t4 = types.Port(choices=[(457, 'ab'), (80, 'xy')])
+        self.assertTrue(t1 == t2 == t3 == t4)
 
     def test_not_equal(self):
         self.assertFalse(types.Port(min=123) == types.Port(min=456))
@@ -973,19 +1001,3 @@ class PortTypeTests(TypeTestHelper, unittest.TestCase):
         t = types.Port(max=0)
         self.assertRaises(ValueError, t, 1)
         t(0)
-
-    def test_with_choices_list(self):
-        t = types.Port(choices=[80, 457])
-        self.assertRaises(ValueError, t, 1)
-        self.assertRaises(ValueError, t, 200)
-        self.assertRaises(ValueError, t, -457)
-        t(80)
-        t(457)
-
-    def test_with_choices_tuple(self):
-        t = types.Port(choices=(80, 457))
-        self.assertRaises(ValueError, t, 1)
-        self.assertRaises(ValueError, t, 200)
-        self.assertRaises(ValueError, t, -457)
-        t(80)
-        t(457)
diff --git a/oslo_config/types.py b/oslo_config/types.py
index 976f497..4462b64 100644
--- a/oslo_config/types.py
+++ b/oslo_config/types.py
@@ -19,6 +19,7 @@ Use these classes as values for the `type` argument to
 
 .. versionadded:: 1.3
 """
+import collections
 import operator
 import re
 import warnings
@@ -68,8 +69,8 @@ class String(ConfigType):
 
     String values do not get transformed and are returned as str objects.
 
-    :param choices: Optional sequence of valid values. Mutually
-                    exclusive with 'regex'.
+    :param choices: Optional sequence of either valid values or tuples of valid
+        values with descriptions. Mutually exclusive with 'regex'.
     :param quotes: If True and string is enclosed with single or double
                    quotes, will strip those quotes. Will signal error if
                    string have quote at the beginning and no quote at
@@ -96,6 +97,10 @@ class String(ConfigType):
     .. versionchanged:: 2.7
        Added *max_length* parameter.
        Added *type_name* parameter.
+
+    .. versionchanged:: 5.2
+       The *choices* parameter will now accept a sequence of tuples, where each
+       tuple is of form (*choice*, *description*)
     """
 
     def __init__(self, choices=None, quotes=False, regex=None,
@@ -109,10 +114,17 @@ class String(ConfigType):
         self.quotes = quotes
         self.max_length = max_length or 0
 
-        self.choices = choices
+        if choices is not None:
+            if not all(isinstance(choice, tuple) for choice in choices):
+                choices = [(choice, None) for choice in choices]
+
+            self.choices = collections.OrderedDict(choices)
+        else:
+            self.choices = None
+
         self.lower_case_choices = None
         if self.choices is not None and self.ignore_case:
-            self.lower_case_choices = [c.lower() for c in choices]
+            self.lower_case_choices = [c.lower() for c in self.choices]
 
         self.regex = regex
         if self.regex is not None:
@@ -146,7 +158,7 @@ class String(ConfigType):
         # Check for case insensitive
         processed_value, choices = ((value.lower(), self.lower_case_choices)
                                     if self.ignore_case else
-                                    (value, self.choices))
+                                    (value, self.choices.keys()))
         if processed_value in choices:
             return value
 
@@ -158,7 +170,7 @@ class String(ConfigType):
     def __repr__(self):
         details = []
         if self.choices is not None:
-            details.append("choices={!r}".format(self.choices))
+            details.append("choices={!r}".format(list(self.choices.keys())))
         if self.regex:
             details.append("regex=%r" % self.regex.pattern)
         if details:
@@ -168,11 +180,12 @@ class String(ConfigType):
     def __eq__(self, other):
         return (
             (self.__class__ == other.__class__) and
-            (set(self.choices) == set(other.choices) if
-             self.choices and other.choices else
-             self.choices == other.choices) and
             (self.quotes == other.quotes) and
-            (self.regex == other.regex)
+            (self.regex == other.regex) and
+            (set([x for x in self.choices or []]) ==
+                set([x for x in other.choices or []]) if
+             self.choices and other.choices else
+             self.choices == other.choices)
         )
 
     def _formatter(self, value):
@@ -252,9 +265,14 @@ class Number(ConfigType):
     :param type_name: Type name to be used in the sample config file.
     :param min: Optional check that value is greater than or equal to min.
     :param max: Optional check that value is less than or equal to max.
-    :param choices: Optional sequence of valid values.
+    :param choices: Optional sequence of either valid values or tuples of valid
+        values with descriptions.
 
     .. versionadded:: 3.14
+
+    .. versionchanged:: 5.2
+       The *choices* parameter will now accept a sequence of tuples, where each
+       tuple is of form (*choice*, *description*)
     """
 
     def __init__(self, num_type, type_name,
@@ -263,15 +281,24 @@ class Number(ConfigType):
 
         if min is not None and max is not None and max < min:
             raise ValueError('Max value is less than min value')
-        invalid_choices = [c for c in choices or []
+
+        if choices is not None:
+            if not all(isinstance(choice, tuple) for choice in choices):
+                choices = [(choice, None) for choice in choices]
+
+            self.choices = collections.OrderedDict(choices)
+        else:
+            self.choices = None
+
+        invalid_choices = [c for c in self.choices or []
                            if (min is not None and min > c)
                            or (max is not None and max < c)]
         if invalid_choices:
             raise ValueError("Choices %s are out of bounds [%s..%s]"
                              % (invalid_choices, min, max))
+
         self.min = min
         self.max = max
-        self.choices = choices
         self.num_type = num_type
 
     def __call__(self, value):
@@ -297,7 +324,7 @@ class Number(ConfigType):
     def __repr__(self):
         props = []
         if self.choices is not None:
-            props.append("choices={!r}".format(self.choices))
+            props.append("choices={!r}".format(list(self.choices.keys())))
         else:
             if self.min is not None:
                 props.append('min=%g' % self.min)
@@ -313,7 +340,8 @@ class Number(ConfigType):
             (self.__class__ == other.__class__) and
             (self.min == other.min) and
             (self.max == other.max) and
-            (set(self.choices) == set(other.choices) if
+            (set([x for x in self.choices or []]) ==
+                set([x for x in other.choices or []]) if
              self.choices and other.choices else
              self.choices == other.choices)
         )
@@ -332,7 +360,8 @@ class Integer(Number):
     :param min: Optional check that value is greater than or equal to min.
     :param max: Optional check that value is less than or equal to max.
     :param type_name: Type name to be used in the sample config file.
-    :param choices: Optional sequence of valid values.
+    :param choices: Optional sequence of either valid values or tuples of valid
+        values with descriptions.
 
     .. versionchanged:: 2.4
        The class now honors zero for *min* and *max* parameters.
@@ -346,6 +375,10 @@ class Integer(Number):
     .. versionchanged:: 3.16
        *choices* is no longer mutually exclusive with *min*/*max*. If those are
        supplied, all choices are verified to be within the range.
+
+    .. versionchanged:: 5.2
+       The *choices* parameter will now accept a sequence of tuples, where each
+       tuple is of form (*choice*, *description*)
     """
 
     def __init__(self, min=None, max=None, type_name='integer value',
@@ -385,9 +418,14 @@ class Port(Integer):
     :param min: Optional check that value is greater than or equal to min.
     :param max: Optional check that value is less than or equal to max.
     :param type_name: Type name to be used in the sample config file.
-    :param choices: Optional sequence of valid values.
+    :param choices: Optional sequence of either valid values or tuples of valid
+        values with descriptions.
 
     .. versionadded:: 3.16
+
+    .. versionchanged:: 5.2
+       The *choices* parameter will now accept a sequence of tuples, where each
+       tuple is of form (*choice*, *description*)
     """
 
     PORT_MIN = 0
diff --git a/releasenotes/notes/support-choice-descriptions-8b2d0c14fbd16b2a.yaml b/releasenotes/notes/support-choice-descriptions-8b2d0c14fbd16b2a.yaml
new file mode 100644
index 0000000..1167381
--- /dev/null
+++ b/releasenotes/notes/support-choice-descriptions-8b2d0c14fbd16b2a.yaml
@@ -0,0 +1,14 @@
+---
+features:
+  - |
+    String, Number, Integer, Float and Port now support value-description
+    tuples in the interable provided for the *choice* parameter. Support for
+    value-only definitions is retained.
+  - |
+    StringOpt and PortOpt now support a value-description tuples in the
+    iterable provided for the *choice* parameter. Support for value-only
+    definitions is retained.
+  - |
+    *oslo-config-generator* and the Sphinx extension will now output
+    descriptions for option choices where provided. This will impact tooling
+    that relies on the *yaml* and *json* output of the former.