Remove hacking check [N347] for config options.
The check for help texts (N347) can be better done by humans. This change removes it. Change-Id: I0861c94f764d559be8f188d3e4b54b2b8ad39ea5
This commit is contained in:
Markus Zoeller
parent
04bc62e4a0
commit
0b0e7dacf5
@ -57,7 +57,6 @@ 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
|
||||
- [N348] Deprecated library function os.popen()
|
||||
- [N349] Check for closures in tests which are not used
|
||||
- [N350] Policy registration should be in the central location ``nova/policies/``
|
||||
|
||||
@ -108,8 +108,6 @@ 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.
|
||||
@ -732,75 +730,6 @@ 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 no_os_popen(logical_line):
|
||||
"""Disallow 'os.popen('
|
||||
|
||||
@ -864,7 +793,6 @@ def factory(register):
|
||||
register(check_python3_no_iteritems)
|
||||
register(check_python3_no_iterkeys)
|
||||
register(check_python3_no_itervalues)
|
||||
register(cfg_help_with_enough_text)
|
||||
register(no_os_popen)
|
||||
register(no_log_warn)
|
||||
register(CheckForUncalledTestClosure)
|
||||
|
||||
@ -657,78 +657,6 @@ 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 parenthesis (weird, but produces a valid string)
|
||||
code7 = """
|
||||
opt = cfg.StrOpt("opt7",
|
||||
help=("help text uses extra parenthesis"))
|
||||
"""
|
||||
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)
|
||||
|
||||
def test_no_os_popen(self):
|
||||
code = """
|
||||
import os
|
||||
|
||||
Loading…
Reference in New Issue
Block a user