diff --git a/HACKING.rst b/HACKING.rst index 3ec9a2371292..397cf5224562 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -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 ------------------- diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 79ed1b676a12..6a0ab2fbdc5d 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -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) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index 8093e677a522..48eff888e7f1 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -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)