diff --git a/HACKING.rst b/HACKING.rst index 0d710b51b69..548a549d8cb 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -8,30 +8,34 @@ Neutron Style Commandments Neutron Specific Commandments ----------------------------- -- [N319] Validate that debug level logs are not translated +Some rules are enforced by `neutron-lib hacking factory +`_ +while other rules are specific to Neutron repository. + +Below you can find a list of checks specific to this repository. + - [N320] Validate that LOG messages, except debug ones, have translations -- [N321] Validate that jsonutils module is used instead of json - [N322] Detect common errors with assert_called_once_with -- [N324] Prevent use of deprecated contextlib.nested. -- [N325] Python 3: Do not use xrange. -- [N326] Python 3: do not use basestring. -- [N327] Python 3: do not use dict.iteritems. - [N328] Detect wrong usage with assertEqual -- [N329] Method's default argument shouldn't be mutable - [N330] Use assertEqual(*empty*, observed) instead of assertEqual(observed, *empty*) - [N331] Detect wrong usage with assertTrue(isinstance()). - [N332] Use assertEqual(expected_http_code, observed_http_code) instead of assertEqual(observed_http_code, expected_http_code). -- [N333] Validate that LOG.warning is used instead of LOG.warn. The latter - is deprecated. - [N334] Use unittest2 uniformly across Neutron. - [N340] Check usage of .i18n (and neutron.i18n) - [N341] Check usage of _ from python builtins - [N343] Production code must not import from neutron.tests.* - [N344] Python 3: Do not use filter(lambda obj: test(obj), data). Replace it with [obj for obj in data if test(obj)]. -- [N345] Detect wrong usage with assertEqual(None, A) and assertEqual(A, None). + +.. note:: + When adding a new hacking check to this repository or ``neutron-lib``, make + sure its number (Nxxx) doesn't clash with any other check. + +.. note:: + As you may have noticed, the numbering for Neutron checks has gaps. This is + because some checks were removed or moved to ``neutron-lib``. Creating Unit Tests ------------------- diff --git a/neutron/hacking/checks.py b/neutron/hacking/checks.py index 9e1dc4e010e..ab3122c061f 100644 --- a/neutron/hacking/checks.py +++ b/neutron/hacking/checks.py @@ -61,8 +61,6 @@ def _regex_for_level(level, hint): } -log_warn = re.compile( - r"(.)*LOG\.(warn)\(\s*('|\"|_)") unittest_imports_dot = re.compile(r"\bimport[\s]+unittest\b") unittest_imports_from = re.compile(r"\bfrom[\s]+unittest\b") filter_match = re.compile(r".*filter\(lambda ") @@ -72,45 +70,6 @@ tests_imports_from1 = re.compile(r"\bfrom[\s]+neutron.tests\b") tests_imports_from2 = re.compile(r"\bfrom[\s]+neutron[\s]+import[\s]+tests\b") -@flake8ext -def use_jsonutils(logical_line, filename): - """N321 - Use jsonutils instead of json.""" - msg = "N321: jsonutils.%(fun)s must be used instead of json.%(fun)s" - - # Some files in the tree are not meant to be run from inside Neutron - # itself, so we should not complain about them not using jsonutils - json_check_skipped_patterns = [ - "neutron/plugins/ml2/drivers/openvswitch/agent/xenapi/etc/xapi.d/" - "plugins/netwrap", - ] - - for pattern in json_check_skipped_patterns: - if pattern in filename: - return - - if "json." in logical_line: - json_funcs = ['dumps(', 'dump(', 'loads(', 'load('] - for f in json_funcs: - pos = logical_line.find('json.%s' % f) - if pos != -1: - yield (pos, msg % {'fun': f[:-1]}) - - -@flake8ext -def no_translate_debug_logs(logical_line, filename): - """N319 - Check for 'LOG.debug(_(' and 'LOG.debug(_Lx(' - - As per our translation policy, - https://wiki.openstack.org/wiki/LoggingStandards#Log_Translation - we shouldn't translate debug level logs. - - * This check assumes that 'LOG' is a logger. - """ - for hint in _all_hints: - if logical_line.startswith("LOG.debug(%s(" % hint): - yield(0, "N319 Don't translate debug level logs") - - @flake8ext def check_assert_called_once_with(logical_line, filename): """N322 - Try to detect unintended calls of nonexistent mock methods like: @@ -136,43 +95,6 @@ def check_assert_called_once_with(logical_line, filename): yield (0, msg) -@flake8ext -def check_no_contextlib_nested(logical_line, filename): - """N324 - Don't use contextlib.nested.""" - msg = ("N324: contextlib.nested is deprecated. With Python 2.7 and later " - "the with-statement supports multiple nested objects. See https://" - "docs.python.org/2/library/contextlib.html#contextlib.nested for " - "more information.") - - if checks.contextlib_nested.match(logical_line): - yield(0, msg) - - -@flake8ext -def check_python3_xrange(logical_line): - """N325 - Do not use xrange.""" - if re.search(r"\bxrange\s*\(", logical_line): - yield(0, "N325: Do not use xrange. Use range, or six.moves.range for " - "large loops.") - - -@flake8ext -def check_no_basestring(logical_line): - """N326 - Don't use basestring.""" - if re.search(r"\bbasestring\b", logical_line): - msg = ("N326: basestring is not Python3-compatible, use " - "six.string_types instead.") - yield(0, msg) - - -@flake8ext -def check_python3_no_iteritems(logical_line): - """N327 - Use six.iteritems()""" - if re.search(r".*\.iteritems\(\)", logical_line): - msg = ("N327: Use six.iteritems() instead of dict.iteritems().") - yield(0, msg) - - @flake8ext def check_asserttruefalse(logical_line, filename): """N328 - Don't use assertEqual(True/False, observed).""" @@ -195,14 +117,6 @@ def check_asserttruefalse(logical_line, filename): yield (0, msg) -@flake8ext -def no_mutable_default_args(logical_line): - """N329 - Don't use mutable default arguments.""" - msg = "N329: Method's default argument shouldn't be mutable!" - if checks.mutable_default_args.match(logical_line): - yield (0, msg) - - @flake8ext def check_assertempty(logical_line, filename): """N330 - Enforce using assertEqual parameter ordering in case of empty @@ -240,14 +154,6 @@ def check_assertequal_for_httpcode(logical_line, filename): yield (0, msg) -@flake8ext -def check_log_warn_deprecated(logical_line, filename): - """N333 - Use LOG.warning.""" - msg = "N333: Use LOG.warning due to compatibility with py3" - if log_warn.match(logical_line): - yield (0, msg) - - @flake8ext def check_oslo_i18n_wrapper(logical_line, filename, noqa): """N340 - Check for neutron.i18n usage. @@ -356,19 +262,6 @@ def check_python3_no_filter(logical_line): yield(0, msg) -@flake8ext -def check_assertIsNone(logical_line, filename): - """N345 - Enforce using assertIsNone.""" - if 'neutron/tests/' in filename: - asse_eq_end_with_none_re = re.compile(r"assertEqual\(.*?,\s+None\)$") - asse_eq_start_with_none_re = re.compile(r"assertEqual\(None,") - res = (asse_eq_start_with_none_re.search(logical_line) or - asse_eq_end_with_none_re.search(logical_line)) - if res: - yield (0, "N345: assertEqual(A, None) or assertEqual(None, A) " - "sentences not allowed") - - @flake8ext def check_no_sqlalchemy_event_import(logical_line, filename, noqa): """N346 - Use neutron.db.api.sqla_listen instead of sqlalchemy event.""" @@ -387,23 +280,15 @@ def check_no_sqlalchemy_event_import(logical_line, filename, noqa): def factory(register): - register(use_jsonutils) + checks.factory(register) register(check_assert_called_once_with) - register(no_translate_debug_logs) - register(check_no_contextlib_nested) - register(check_python3_xrange) - register(check_no_basestring) - register(check_python3_no_iteritems) register(check_asserttruefalse) - register(no_mutable_default_args) register(check_assertempty) register(check_assertisinstance) register(check_assertequal_for_httpcode) - register(check_log_warn_deprecated) register(check_oslo_i18n_wrapper) register(check_builtins_gettext) register(check_unittest_imports) register(check_no_imports_from_tests) register(check_python3_no_filter) - register(check_assertIsNone) register(check_no_sqlalchemy_event_import) diff --git a/neutron/tests/unit/hacking/test_checks.py b/neutron/tests/unit/hacking/test_checks.py index ec634b5e9ca..5b0914b3673 100644 --- a/neutron/tests/unit/hacking/test_checks.py +++ b/neutron/tests/unit/hacking/test_checks.py @@ -36,38 +36,6 @@ class HackingTestCase(base.BaseTestCase): def assertLineFails(self, func, line): self.assertIsInstance(next(func(line)), tuple) - def test_no_translate_debug_logs(self): - for hint in checks._all_hints: - bad = "LOG.debug(%s('bad'))" % hint - self.assertEqual( - 1, len(list(checks.no_translate_debug_logs(bad, 'f')))) - - def test_use_jsonutils(self): - def __get_msg(fun): - msg = ("N321: jsonutils.%(fun)s must be used instead of " - "json.%(fun)s" % {'fun': fun}) - return [(0, msg)] - - for method in ('dump', 'dumps', 'load', 'loads'): - self.assertEqual( - __get_msg(method), - list(checks.use_jsonutils("json.%s(" % method, - "./neutron/common/rpc.py"))) - - self.assertEqual(0, - len(list(checks.use_jsonutils("jsonx.%s(" % method, - "./neutron/common/rpc.py")))) - - self.assertEqual(0, - len(list(checks.use_jsonutils("json.%sx(" % method, - "./neutron/common/rpc.py")))) - - self.assertEqual(0, - len(list(checks.use_jsonutils( - "json.%s" % method, - "./neutron/plugins/ml2/drivers/openvswitch/agent/xenapi/" - "etc/xapi.d/plugins/netwrap")))) - def test_assert_called_once_with(self): fail_code1 = """ mock = Mock() @@ -118,36 +86,6 @@ class HackingTestCase(base.BaseTestCase): 0, len(list(checks.check_assert_called_once_with(pass_code2, "neutron/tests/test_assert.py")))) - def test_check_python3_xrange(self): - f = checks.check_python3_xrange - self.assertLineFails(f, 'a = xrange(1000)') - self.assertLineFails(f, 'b =xrange ( 42 )') - self.assertLineFails(f, 'c = xrange(1, 10, 2)') - self.assertLinePasses(f, 'd = range(1000)') - self.assertLinePasses(f, 'e = six.moves.range(1337)') - - def test_no_basestring(self): - self.assertEqual(1, - len(list(checks.check_no_basestring("isinstance(x, basestring)")))) - - def test_check_python3_iteritems(self): - f = checks.check_python3_no_iteritems - self.assertLineFails(f, "d.iteritems()") - self.assertLinePasses(f, "six.iteritems(d)") - - def test_no_mutable_default_args(self): - self.assertEqual(1, len(list(checks.no_mutable_default_args( - " def fake_suds_context(calls={}):")))) - - self.assertEqual(1, len(list(checks.no_mutable_default_args( - "def get_info_from_bdm(virt_type, bdm, mapping=[])")))) - - self.assertEqual(0, len(list(checks.no_mutable_default_args( - "defined = []")))) - - self.assertEqual(0, len(list(checks.no_mutable_default_args( - "defined, undefined = [], {}")))) - def test_asserttruefalse(self): true_fail_code1 = """ test_bool = True @@ -268,11 +206,6 @@ class HackingTestCase(base.BaseTestCase): self.assertLineFails(f, 'from unittest.TestSuite') self.assertLineFails(f, 'import unittest') - def test_check_log_warn_deprecated(self): - bad = "LOG.warn(_LW('i am zlatan!'))" - self.assertEqual( - 1, len(list(checks.check_log_warn_deprecated(bad, 'f')))) - def test_check_no_imports_from_tests(self): fail_codes = ('from neutron import tests', 'from neutron.tests import base', @@ -294,16 +227,6 @@ class HackingTestCase(base.BaseTestCase): self.assertLinePasses(f, "filter(function, range(0,10))") self.assertLinePasses(f, "lambda x, y: x+y") - def test_check_assertIsNone(self): - self.assertEqual(1, len(list(checks.check_assertIsNone( - "self.assertEqual(A, None)", "neutron/tests/test_assert.py")))) - - self.assertEqual(1, len(list(checks.check_assertIsNone( - "self.assertEqual(None, A)", "neutron/tests/test_assert.py")))) - - self.assertEqual(0, len(list(checks.check_assertIsNone( - "self.assertIsNone()", "neutron/tests/test_assert.py")))) - # The following is borrowed from hacking/tests/test_doctest.py. # Tests defined in docstring is easier to understand @@ -332,6 +255,7 @@ class HackingDocTestCase(hacking_doctest.HackingTestCase): if self.options.select: turn_on.update(self.options.select) self.options.select = tuple(turn_on) + self.options.ignore = ('N530',) report = pep8.BaseReport(self.options) checker = pep8.Checker(filename=self.filename, lines=self.lines, diff --git a/tox.ini b/tox.ini index 61b234b2c0b..af142cd4068 100644 --- a/tox.ini +++ b/tox.ini @@ -119,11 +119,15 @@ commands = sphinx-build -W -b html doc/source doc/build/html # E265 block comment should start with '# ' # H404 multi line docstring should start with a summary # H405 multi line docstring summary not separated with an empty line -ignore = E125,E126,E128,E129,E265,H404,H405 +# N530 direct neutron imports not allowed +# N534 Untranslated exception message +# N536 Use assertIsNone rather than assertEqual to check for None values +# TODO(ihrachys) figure out what to do with N534 and N536 +ignore = E125,E126,E128,E129,E265,H404,H405,N530,N534,N536 # H904: Delay string interpolations at logging calls enable-extensions=H904 show-source = true -exclude = ./.*,build,dist +exclude = ./.*,build,dist,doc [hacking] import_exceptions = neutron._i18n