diff --git a/.gitignore b/.gitignore index 963e589..4f3911d 100644 --- a/.gitignore +++ b/.gitignore @@ -55,4 +55,7 @@ ChangeLog .*sw? # Files created by releasenotes build -releasenotes/build \ No newline at end of file +releasenotes/build + +# PyCharm project directory +.idea diff --git a/octaviaclient/hacking/__init__.py b/octaviaclient/hacking/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/octaviaclient/hacking/checks.py b/octaviaclient/hacking/checks.py new file mode 100644 index 0000000..f38c72d --- /dev/null +++ b/octaviaclient/hacking/checks.py @@ -0,0 +1,263 @@ +# Copyright (c) 2014 OpenStack Foundation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import re + + +""" +Guidelines for writing new hacking checks + + - Use only for octaviaclient specific tests. OpenStack general tests + should be submitted to the common 'hacking' module. + - Pick numbers in the range O3xx. Find the current test with + the highest allocated number and then pick the next value. + - Keep the test method code in the source file ordered based + on the O3xx value. + - List the new rule in the top level HACKING.rst file + - Add test cases for each new rule to + octaviaclient/tests/unit/test_hacking.py + +""" + +author_tag_re = (re.compile("^\s*#\s*@?(a|A)uthor"), + re.compile("^\.\.\s+moduleauthor::")) + +_all_log_levels = {'critical', 'error', 'exception', 'info', 'warning'} +_all_hints = {'_LC', '_LE', '_LI', '_', '_LW'} + +_log_translation_hint = re.compile( + r".*LOG\.(%(levels)s)\(\s*(%(hints)s)\(" % { + 'levels': '|'.join(_all_log_levels), + 'hints': '|'.join(_all_hints), + }) + +assert_trueinst_re = re.compile( + r"(.)*assertTrue\(isinstance\((\w|\.|\'|\"|\[|\])+, " + "(\w|\.|\'|\"|\[|\])+\)\)") +assert_equal_in_end_with_true_or_false_re = re.compile( + r"assertEqual\((\w|[][.'\"])+ in (\w|[][.'\", ])+, (True|False)\)") +assert_equal_in_start_with_true_or_false_re = re.compile( + r"assertEqual\((True|False), (\w|[][.'\"])+ in (\w|[][.'\", ])+\)") +assert_equal_with_true_re = re.compile( + r"assertEqual\(True,") +assert_equal_with_false_re = re.compile( + r"assertEqual\(False,") +mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") +assert_equal_end_with_none_re = re.compile(r"(.)*assertEqual\(.+, None\)") +assert_equal_start_with_none_re = re.compile(r".*assertEqual\(None, .+\)") +assert_not_equal_end_with_none_re = re.compile( + r"(.)*assertNotEqual\(.+, None\)") +assert_not_equal_start_with_none_re = re.compile( + r"(.)*assertNotEqual\(None, .+\)") +assert_no_xrange_re = re.compile( + r"\s*xrange\s*\(") + + +def _translation_checks_not_enforced(filename): + # Do not do these validations on tests + return any(pat in filename for pat in ["/tests/", "rally-jobs/plugins/"]) + + +def assert_true_instance(logical_line): + """Check for assertTrue(isinstance(a, b)) sentences + + O316 + """ + if assert_trueinst_re.match(logical_line): + yield (0, "O316: assertTrue(isinstance(a, b)) sentences not allowed") + + +def assert_equal_or_not_none(logical_line): + """Check for assertEqual(A, None) or assertEqual(None, A) sentences, + + assertNotEqual(A, None) or assertNotEqual(None, A) sentences + + O318 + """ + msg = ("O318: assertEqual/assertNotEqual(A, None) or " + "assertEqual/assertNotEqual(None, A) sentences not allowed") + res = (assert_equal_start_with_none_re.match(logical_line) or + assert_equal_end_with_none_re.match(logical_line) or + assert_not_equal_start_with_none_re.match(logical_line) or + assert_not_equal_end_with_none_re.match(logical_line)) + if res: + yield (0, msg) + + +def no_author_tags(physical_line): + for regex in author_tag_re: + if regex.match(physical_line): + physical_line = physical_line.lower() + pos = physical_line.find('moduleauthor') + if pos < 0: + pos = physical_line.find('author') + return pos, "O322: Don't use author tags" + + +def assert_equal_true_or_false(logical_line): + """Check for assertEqual(True, A) or assertEqual(False, A) sentences + + O323 + """ + res = (assert_equal_with_true_re.search(logical_line) or + assert_equal_with_false_re.search(logical_line)) + if res: + yield (0, "O323: assertEqual(True, A) or assertEqual(False, A) " + "sentences not allowed") + + +def no_mutable_default_args(logical_line): + msg = "O324: Method's default argument shouldn't be mutable!" + if mutable_default_args.match(logical_line): + yield (0, msg) + + +def assert_equal_in(logical_line): + """Check for assertEqual(A in B, True), assertEqual(True, A in B), + + assertEqual(A in B, False) or assertEqual(False, A in B) sentences + + O338 + """ + res = (assert_equal_in_start_with_true_or_false_re.search(logical_line) or + assert_equal_in_end_with_true_or_false_re.search(logical_line)) + if res: + yield (0, "O338: Use assertIn/NotIn(A, B) rather than " + "assertEqual(A in B, True/False) when checking collection " + "contents.") + + +def no_log_warn(logical_line): + """Disallow 'LOG.warn(' + + O339 + """ + if logical_line.startswith('LOG.warn('): + yield(0, "O339:Use LOG.warning() rather than LOG.warn()") + + +def no_xrange(logical_line): + """Disallow 'xrange()' + + O340 + """ + if assert_no_xrange_re.match(logical_line): + yield(0, "O340: Do not use xrange().") + + +def no_translate_logs(logical_line, filename): + """O341 - Don't translate logs. + + Check for 'LOG.*(_(' and 'LOG.*(_Lx(' + + Translators don't provide translations for log messages, and operators + asked not to translate them. + + * This check assumes that 'LOG' is a logger. + + :param logical_line: The logical line to check. + :param filename: The file name where the logical line exists. + :returns: None if the logical line passes the check, otherwise a tuple + is yielded that contains the offending index in logical line and a + message describe the check validation failure. + """ + if _translation_checks_not_enforced(filename): + return + + msg = "O341: Log messages should not be translated!" + match = _log_translation_hint.match(logical_line) + if match: + yield (logical_line.index(match.group()), msg) + + +def check_raised_localized_exceptions(logical_line, filename): + """O342 - Untranslated exception message. + + :param logical_line: The logical line to check. + :param filename: The file name where the logical line exists. + :returns: None if the logical line passes the check, otherwise a tuple + is yielded that contains the offending index in logical line and a + message describe the check validation failure. + """ + if _translation_checks_not_enforced(filename): + return + + logical_line = logical_line.strip() + raised_search = re.compile( + r"raise (?:\w*)\((.*)\)").match(logical_line) + if raised_search: + exception_msg = raised_search.groups()[0] + if exception_msg.startswith("\"") or exception_msg.startswith("\'"): + msg = "O342: Untranslated exception message." + yield (logical_line.index(exception_msg), msg) + + +def check_no_basestring(logical_line): + """O343 - basestring is not Python3-compatible. + + :param logical_line: The logical line to check. + :returns: None if the logical line passes the check, otherwise a tuple + is yielded that contains the offending index in logical line and a + message describe the check validation failure. + """ + if re.search(r"\bbasestring\b", logical_line): + msg = ("O343: basestring is not Python3-compatible, use " + "six.string_types instead.") + yield(0, msg) + + +def check_python3_no_iteritems(logical_line): + """O344 - Use dict.items() instead of dict.iteritems(). + + :param logical_line: The logical line to check. + :returns: None if the logical line passes the check, otherwise a tuple + is yielded that contains the offending index in logical line and a + message describe the check validation failure. + """ + if re.search(r".*\.iteritems\(\)", logical_line): + msg = ("O344: Use dict.items() instead of dict.iteritems() to be " + "compatible with both Python 2 and Python 3. In Python 2, " + "dict.items() may be inefficient for very large dictionaries. " + "If you can prove that you need the optimization of an " + "iterator for Python 2, then you can use six.iteritems(dict).") + yield(0, msg) + + +def check_no_eventlet_imports(logical_line): + """O345 - Usage of Python eventlet module not allowed. + + :param logical_line: The logical line to check. + :returns: None if the logical line passes the check, otherwise a tuple + is yielded that contains the offending index in logical line and a + message describe the check validation failure. + """ + if re.match(r'(import|from)\s+[(]?eventlet', logical_line): + msg = 'O345 Usage of Python eventlet module not allowed' + yield logical_line.index('eventlet'), msg + + +def factory(register): + register(assert_true_instance) + register(assert_equal_or_not_none) + register(no_translate_logs) + register(no_author_tags) + register(assert_equal_true_or_false) + register(no_mutable_default_args) + register(assert_equal_in) + register(no_log_warn) + register(no_xrange) + register(check_raised_localized_exceptions) + register(check_no_basestring) + register(check_python3_no_iteritems) + register(check_no_eventlet_imports) diff --git a/octaviaclient/tests/unit/test_hacking.py b/octaviaclient/tests/unit/test_hacking.py new file mode 100644 index 0000000..2a7990e --- /dev/null +++ b/octaviaclient/tests/unit/test_hacking.py @@ -0,0 +1,144 @@ +# Copyright 2015 +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslotest import base + +from octaviaclient.hacking import checks + + +class HackingTestCase(base.BaseTestCase): + """Hacking test class. + + This class tests the hacking checks in octaviaclient.hacking.checks by + passing strings to the check methods like the pep8/flake8 parser would. The + parser loops over each line in the file and then passes the parameters to + the check method. The parameter names in the check method dictate what type + of object is passed to the check method. The parameter types are:: + + logical_line: A processed line with the following modifications: + - Multi-line statements converted to a single line. + - Stripped left and right. + - Contents of strings replaced with "xxx" of same length. + - Comments removed. + physical_line: Raw line of text from the input file. + lines: a list of the raw lines from the input file + tokens: the tokens that contribute to this logical line + line_number: line number in the input file + total_lines: number of lines in the input file + blank_lines: blank lines before this one + indent_char: indentation character in this file (" " or "\t") + indent_level: indentation (with tabs expanded to multiples of 8) + previous_indent_level: indentation on previous line + previous_logical: previous logical line + filename: Path of the file being run through pep8 + + When running a test on a check method the return will be False/None if + there is no violation in the sample input. If there is an error a tuple is + returned with a position in the line, and a message. So to check the result + just assertTrue if the check is expected to fail and assertFalse if it + should pass. + """ + + def test_assert_true_instance(self): + self.assertEqual(1, len(list(checks.assert_true_instance( + "self.assertTrue(isinstance(e, " + "exception.BuildAbortException))")))) + + self.assertEqual(0, len(list(checks.assert_true_instance( + "self.assertTrue()")))) + + def test_assert_equal_or_not_none(self): + self.assertEqual(1, len(list(checks.assert_equal_or_not_none( + "self.assertEqual(A, None)")))) + + self.assertEqual(1, len(list(checks.assert_equal_or_not_none( + "self.assertEqual(None, A)")))) + + self.assertEqual(1, len(list(checks.assert_equal_or_not_none( + "self.assertNotEqual(A, None)")))) + + self.assertEqual(1, len(list(checks.assert_equal_or_not_none( + "self.assertNotEqual(None, A)")))) + + self.assertEqual(0, + len(list(checks.assert_equal_or_not_none( + "self.assertIsNone()")))) + + self.assertEqual(0, + len(list(checks.assert_equal_or_not_none( + "self.assertIsNotNone()")))) + + def test_assert_equal_in(self): + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(a in b, True)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual('str' in 'string', True)")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(any(a==1 for a in b), True)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(True, a in b)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(True, 'str' in 'string')")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(True, any(a==1 for a in b))")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(a in b, False)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual('str' in 'string', False)")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(any(a==1 for a in b), False)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(False, a in b)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(False, 'str' in 'string')")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(False, any(a==1 for a in b))")))) + + def test_assert_equal_true_or_false(self): + self.assertEqual(1, len(list(checks.assert_equal_true_or_false( + "self.assertEqual(True, A)")))) + + self.assertEqual(1, len(list(checks.assert_equal_true_or_false( + "self.assertEqual(False, A)")))) + + self.assertEqual(0, len(list(checks.assert_equal_true_or_false( + "self.assertTrue()")))) + + self.assertEqual(0, len(list(checks.assert_equal_true_or_false( + "self.assertFalse()")))) + + def test_no_log_warn(self): + self.assertEqual(1, len(list(checks.no_log_warn( + "LOG.warn()")))) + + self.assertEqual(0, len(list(checks.no_log_warn( + "LOG.warning()")))) + + def test_no_xrange(self): + self.assertEqual(1, len(list(checks.no_xrange( + "xrange(45)")))) + + self.assertEqual(0, len(list(checks.no_xrange( + "range(45)")))) diff --git a/releasenotes/source/conf.py b/releasenotes/source/conf.py index c4546fc..51f88ee 100644 --- a/releasenotes/source/conf.py +++ b/releasenotes/source/conf.py @@ -207,7 +207,7 @@ latex_elements = { # Grouping the document tree into LaTeX files. List of tuples # (source start file, target name, title, -# author, documentclass [howto, manual, or own class]). +# documentclass [howto, manual, or own class]). latex_documents = [ ('index', 'GlanceReleaseNotes.tex', u'Glance Release Notes Documentation', u'Glance Developers', 'manual'), diff --git a/tox.ini b/tox.ini index 777e784..aae7a0f 100644 --- a/tox.ini +++ b/tox.ini @@ -36,8 +36,10 @@ commands = [flake8] # E123, E125 skipped as they are invalid PEP-8. - -show-source = True ignore = E123,E125 +show-source = true builtins = _ exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build + +[hacking] +local-check-factory = octaviaclient.hacking.checks.factory