openstack
/
nova
Code Issues Proposed changes

config options: add hacking check for help text length 

Adds a hacking check if a config option provides enough help text.
To do so, the check counts the length of the help text. It uses
a low number in order to avoid a break. As soon as we have agreed
on a higher standard and changed the options accordingly, we can
increase that number.

Change-Id: If31339c428953c6a7bf721ff92e5999e8cb5b569
This commit is contained in:
Markus Zoeller 2015-11-05 15:14:29 +01:00
parent 11dfd9def3
commit 1216449dc4
3 changed files with 145 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
HACKING.rst
View File

@ -57,6 +57,7 @@ Nova Specific Commandments
- [N344] Python 3: do not use dict.iteritems.
- [N345] Python 3: do not use dict.iterkeys.
- [N346] Python 3: do not use dict.itervalues.
- [N347] Provide enough help text for config options
Creating Unit Tests
-------------------

72
nova/hacking/checks.py
View File

@ -104,6 +104,8 @@ contextlib_nested = re.compile(r"^with (contextlib\.)?nested\(")
doubled_words_re = re.compile(
r"\b(then?|[iao]n|i[fst]|but|f?or|at|and|[dt]o)\s+\1\b")
opt_help_text_min_char_count = 10
class BaseASTChecker(ast.NodeVisitor):
"""Provides a simple framework for writing AST-based checks.
@ -612,6 +614,75 @@ def check_python3_no_itervalues(logical_line):
yield(0, msg)
def cfg_help_with_enough_text(logical_line, tokens):
# TODO(markus_z): The count of 10 chars is the *highest* number I could
# use to introduce this new check without breaking the gate. IOW, if I
# use a value of 15 for example, the gate checks will fail because we have
# a few config options which use fewer chars than 15 to explain their
# usage (for example the options "ca_file" and "cert").
# As soon as the implementation of bp centralize-config-options is
# finished, I wanted to increase that magic number to a higher (to be
# defined) value.
# This check is an attempt to programmatically check a part of the review
# guidelines http://docs.openstack.org/developer/nova/code-review.html
msg = ("N347: A config option is a public interface to the cloud admins "
"and should be properly documented. A part of that is to provide "
"enough help text to describe this option. Use at least %s chars "
"for that description. Is is likely that this minimum will be "
"increased in the future." % opt_help_text_min_char_count)
if not cfg_opt_re.match(logical_line):
return
# ignore DeprecatedOpt objects. They get mentioned in the release notes
# and don't need a lengthy help text anymore
if "DeprecatedOpt" in logical_line:
return
def get_token_value(idx):
return tokens[idx][1]
def get_token_values(start_index, length):
values = ""
for offset in range(length):
values += get_token_value(start_index + offset)
return values
def get_help_token_index():
for idx in range(len(tokens)):
if get_token_value(idx) == "help":
return idx
return -1
def has_help():
return get_help_token_index() >= 0
def get_trimmed_help_text(t):
txt = ""
# len(["help", "=", "_", "("]) ==> 4
if get_token_values(t, 4) == "help=_(":
txt = get_token_value(t + 4)
# len(["help", "=", "("]) ==> 3
elif get_token_values(t, 3) == "help=(":
txt = get_token_value(t + 3)
# len(["help", "="]) ==> 2
else:
txt = get_token_value(t + 2)
return " ".join(txt.strip('\"\'').split())
def has_enough_help_text(txt):
return len(txt) >= opt_help_text_min_char_count
if has_help():
t = get_help_token_index()
txt = get_trimmed_help_text(t)
if not has_enough_help_text(txt):
yield(0, msg)
else:
yield(0, msg)
def factory(register):
register(import_no_db_in_virt)
register(no_db_session_in_public_api)
@ -645,3 +716,4 @@ def factory(register):
register(check_python3_no_iteritems)
register(check_python3_no_iterkeys)
register(check_python3_no_itervalues)
register(cfg_help_with_enough_text)

72
nova/tests/unit/test_hacking.py
View File

@ -660,3 +660,75 @@ class HackingTestCase(test.NoDBTestCase):
self.assertEqual(0, len(list(checks.check_python3_no_itervalues(
"six.itervalues(ob))"))))
def test_cfg_help_with_enough_text(self):
errors = [(1, 0, 'N347')]
# Doesn't have help text at all => should raise error
code1 = """
opt = cfg.StrOpt("opt1")
"""
self._assert_has_errors(code1, checks.cfg_help_with_enough_text,
expected_errors=errors)
# Explicitly sets an empty string => should raise error
code2 = """
opt = cfg.StrOpt("opt2", help="")
"""
self._assert_has_errors(code2, checks.cfg_help_with_enough_text,
expected_errors=errors)
# Has help text but too few characters => should raise error
code3 = """
opt = cfg.StrOpt("opt3", help="meh")
"""
self._assert_has_errors(code3, checks.cfg_help_with_enough_text,
expected_errors=errors)
# Has long enough help text => should *not* raise an error
code4 = """
opt = cfg.StrOpt("opt4", help="This option does stuff")
"""
self._assert_has_no_errors(code4, checks.cfg_help_with_enough_text)
# OptGroup objects help is optional => should *not* raise error
code5 = """
opt_group = cfg.OptGroup(name="group1", title="group title")
"""
self._assert_has_no_errors(code5, checks.cfg_help_with_enough_text)
# The help text gets translated
code6 = """
opt = cfg.StrOpt("opt6",
help=_("help with translation usage"))
"""
self._assert_has_no_errors(code6, checks.cfg_help_with_enough_text)
# The help text uses a paranthesis (weird, but produces a valid string)
code7 = """
opt = cfg.StrOpt("opt7",
help=("help text uses extra paranthesis"))
"""
self._assert_has_no_errors(code7, checks.cfg_help_with_enough_text)
# Ignore deprecated options. They should be in the release notes
code8 = """
opt = cfg.DeprecatedOpt('opt8')
"""
self._assert_has_no_errors(code8, checks.cfg_help_with_enough_text)
code9 = """
opt = cfg.StrOpt("opt9",
help=\"\"\"
This
is
multiline
help
text.
\"\"\")
"""
self._assert_has_no_errors(code9, checks.cfg_help_with_enough_text)