openstack/oslo.config
Code Issues Proposed changes

Provide descriptions for choices

Browse Source 
Nova uses a common pattern for choices where a 'choices' parameter is
provided and the choices are later documented in the help text. This
frequently leads to code and docs getting out-of-sync and requires
authors to be consistent in how they write option descriptions.
Eliminate the need to do this by allowing users to describe the choices
in the same place as the choices are declared.

Change-Id: Ic084b04ebf232fb72c9c05bbea3a216391b15c83
This commit is contained in:
Stephen Finucane 2017-12-05 15:02:49 +00:00
parent a6a4f279ef
commit bc9b7f5d2f
8 changed files with 227 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
oslo_config/cfg.py
View File

@ -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):

38
oslo_config/generator.py
View File

@ -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'])

12
oslo_config/sphinxext.py
View File

@ -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'],

15
oslo_config/tests/test_generator.py
View File

@ -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': [],

45
oslo_config/tests/test_sphinxext.py
View File

@ -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(

82
oslo_config/tests/test_types.py
View File

@ -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)

72
oslo_config/types.py
View File

@ -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

14
releasenotes/notes/support-choice-descriptions-8b2d0c14fbd16b2a.yaml Normal file
View File

@ -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.