diff --git a/HACKING.rst b/HACKING.rst index 8f61c0086281..8c09f530a014 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -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/`` diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 739cbadb7801..b60e87b5da4d 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -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) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index 31a0bc176ca6..774a6d06443a 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -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