From ec6d5d4718e47e6f3e05a959fa348d3ca01efb7f Mon Sep 17 00:00:00 2001 From: David Stanek Date: Wed, 5 Mar 2014 07:04:41 +0000 Subject: [PATCH] Adds style checks to ease reviewer burden The following checks were added in this commit: - mutable default args are no longer allowed - block comments must begin with at least one space - enforce assertIsNone instead of assertEqual with a None bp more-code-style-automation Change-Id: I7dba0c8b45078a665d40993d751c79566aa7505f --- keystone/hacking/__init__.py | 0 keystone/hacking/checks.py | 141 ++++++++++++++++++++++++++ keystone/tests/test_backend.py | 6 +- keystone/tests/test_hacking_checks.py | 133 ++++++++++++++++++++++++ tox.ini | 1 + 5 files changed, 278 insertions(+), 3 deletions(-) create mode 100644 keystone/hacking/__init__.py create mode 100644 keystone/hacking/checks.py create mode 100644 keystone/tests/test_hacking_checks.py diff --git a/keystone/hacking/__init__.py b/keystone/hacking/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/keystone/hacking/checks.py b/keystone/hacking/checks.py new file mode 100644 index 0000000000..33f572ae96 --- /dev/null +++ b/keystone/hacking/checks.py @@ -0,0 +1,141 @@ +# 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. + +"""Keystone's pep8 extensions. + +In order to make the review process faster and easier for core devs we are +adding some Keystone specific pep8 checks. This will catch common errors +so that core devs don't have to. + +There are two types of pep8 extensions. One is a function that takes either +a physical or logical line. The physical or logical line is the first param +in the function definition and can be followed by other parameters supported +by pep8. The second type is a class that parses AST trees. For more info +please see pep8.py. +""" + +import ast + + +class BaseASTChecker(ast.NodeVisitor): + """Provides a simple framework for writing AST-based checks. + + Subclasses should implement visit_* methods like any other AST visitor + implementation. When they detect an error for a particular node the + method should call ``self.add_error(offending_node)``. Details about + where in the code the error occured will be pulled from the node + object. + + Subclasses should also provide a class variable named CHECK_DESC to + be used for the human readable error message. + + """ + + def __init__(self, tree, filename): + """This object is created automatically by pep8. + + :param tree: an AST tree + :param filename: name of the file being analyzed + (ignored by our checks) + """ + self._tree = tree + self._errors = [] + + def run(self): + """Called automatically by pep8.""" + self.visit(self._tree) + return self._errors + + def add_error(self, node, message=None): + """Add an error caused by a node to the list of errors for pep8.""" + message = message or self.CHECK_DESC + error = (node.lineno, node.col_offset, message, self.__class__) + self._errors.append(error) + + +class CheckForMutableDefaultArgs(BaseASTChecker): + """Checks for the use of mutable objects as function/method defaults. + + We are only checking for list and dict literals at this time. This means + that a developer could specify an instance of their own and cause a bug. + The fix for this is probably more work than it's worth because it will + get caught during code review. + + """ + + CHECK_DESC = 'K001 Using mutable as a function/method default' + + # TODO(dstanek): once we drop support for Python 2.6 we can add ast.Set, + # ast.DictComp and ast.SetComp to MUTABLES. Don't forget the tests! + MUTABLES = (ast.List, ast.ListComp, ast.Dict) + + def visit_FunctionDef(self, node): + for arg in node.args.defaults: + if isinstance(arg, self.MUTABLES): + self.add_error(arg) + + super(CheckForMutableDefaultArgs, self).generic_visit(node) + + +def block_comments_begin_with_a_space(physical_line, line_number): + """There should be a space after the # of block comments. + + There is already a check in pep8 that enforces this rule for + inline comments. + + Okay: # this is a comment + Okay: #!/usr/bin/python + Okay: # this is a comment + K002: #this is a comment + + """ + MESSAGE = "K002 block comments should start with '# '" + + # shebangs are OK + if line_number == 1 and physical_line.startswith('#!'): + return + + text = physical_line.strip() + if text.startswith('#'): # look for block comments + if len(text) > 1 and not text[1].isspace(): + return physical_line.index('#'), MESSAGE + + +class CheckForAssertingNoneEquality(BaseASTChecker): + """Ensures that code does not use a None with assert(Not*)Equal.""" + + CHECK_DESC_IS = ('K003 Use self.assertIsNone(...) when comparing ' + 'against None') + CHECK_DESC_ISNOT = ('K004 Use assertIsNotNone(...) when comparing ' + ' against None') + + def visit_Call(self, node): + # NOTE(dstanek): I wrote this in a verbose way to make it easier to + # read for those that have little experience with Python's AST. + + if isinstance(node.func, ast.Attribute): + if node.func.attr == 'assertEqual': + for arg in node.args: + if isinstance(arg, ast.Name) and arg.id == 'None': + self.add_error(node, message=self.CHECK_DESC_IS) + elif node.func.attr == 'assertNotEqual': + for arg in node.args: + if isinstance(arg, ast.Name) and arg.id == 'None': + self.add_error(node, message=self.CHECK_DESC_ISNOT) + + super(CheckForAssertingNoneEquality, self).generic_visit(node) + + +def factory(register): + register(CheckForMutableDefaultArgs) + register(block_comments_begin_with_a_space) + register(CheckForAssertingNoneEquality) diff --git a/keystone/tests/test_backend.py b/keystone/tests/test_backend.py index ea5d264c8a..cf3b9695e4 100644 --- a/keystone/tests/test_backend.py +++ b/keystone/tests/test_backend.py @@ -2346,8 +2346,8 @@ class IdentityTests(object): group_refs = self.identity_api.list_groups_for_user( positive_user['id']) self.assertEqual(after_count, len(group_refs)) - #Make sure the group count for the unrelated user - #did not change + # Make sure the group count for the unrelated user + # did not change group_refs = self.identity_api.list_groups_for_user( negative_user['id']) self.assertEqual(0, len(group_refs)) @@ -4199,7 +4199,7 @@ class FilterTests(filtering.FilterTests): self.assertEqual(2, len(users)) self.assertEqual(user_list[7]['id'], users[0]['id']) self.assertEqual(user_list[10]['id'], users[1]['id']) - #TODO(henry-nash) Check inexact filter has been removed. + # TODO(henry-nash) Check inexact filter has been removed. # TODO(henry-nash): Add some case sensitive tests. The issue # is that MySQL 0.7, by default, is installed in case diff --git a/keystone/tests/test_hacking_checks.py b/keystone/tests/test_hacking_checks.py new file mode 100644 index 0000000000..a94108dc77 --- /dev/null +++ b/keystone/tests/test_hacking_checks.py @@ -0,0 +1,133 @@ +# 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 textwrap + +import mock +import pep8 +import testtools + +from keystone.hacking import checks + + +class BaseStyleCheck(testtools.TestCase): + + def get_checker(self): + """Returns the checker to be used for tests in this class.""" + raise NotImplemented('subclasses must provide a real implementation') + + # We are patching pep8 so that only the check under test is actually + # installed. + @mock.patch('pep8._checks', + {'physical_line': {}, 'logical_line': {}, 'tree': {}}) + def run_check(self, code): + pep8.register_check(self.get_checker()) + + lines = textwrap.dedent(code).strip().splitlines(True) + + checker = pep8.Checker(lines=lines) + checker.check_all() + checker.report._deferred_print.sort() + return checker.report._deferred_print + + def assert_has_errors(self, code, expected_errors=None): + actual_errors = [e[:3] for e in self.run_check(code)] + self.assertEqual(expected_errors or [], actual_errors) + + +class TestCheckForMutableDefaultArgs(BaseStyleCheck): + + def get_checker(self): + return checks.CheckForMutableDefaultArgs + + def test(self): + code = """ + def f(): + pass + + def f(a, b='', c=None): + pass + + def f(bad=[]): + pass + + def f(foo, bad=[], more_bad=[x for x in range(3)]): + pass + + def f(foo, bad={}): + pass + + def f(foo, bad={}, another_bad=[], fine=None): + pass + + def f(bad=[]): # noqa + pass + + """ + expected_errors = [ + (7, 10, 'K001'), + (10, 15, 'K001'), + (10, 29, 'K001'), + (13, 15, 'K001'), + (16, 15, 'K001'), + (16, 31, 'K001'), + ] + self.assert_has_errors(code, expected_errors=expected_errors) + + +class TestBlockCommentsBeginWithASpace(BaseStyleCheck): + + def get_checker(self): + return checks.block_comments_begin_with_a_space + + def test(self): + # NOTE(dstanek): The 'noqa' line below will stop the normal CI + # pep8 process from flaging an error when running against this code. + # The unit tests use pep8 directly and the 'noqa' has no effect so we + # can easilty test. + code = """ + # This is a good comment + + #This is a bad one # flake8: noqa + + # This is alright and can + # be continued with extra indentation + # if that's what the developer wants. + """ + self.assert_has_errors(code, [(3, 0, 'K002')]) + + +class TestAssertingNoneEquality(BaseStyleCheck): + + def get_checker(self): + return checks.CheckForAssertingNoneEquality + + def test(self): + code = """ + class Test(object): + + def test(self): + self.assertEqual('', '') + self.assertEqual('', None) + self.assertEqual(None, '') + self.assertNotEqual('', None) + self.assertNotEqual(None, '') + self.assertNotEqual('', None) # noqa + self.assertNotEqual(None, '') # noqa + """ + expected_errors = [ + (5, 8, 'K003'), + (6, 8, 'K003'), + (7, 8, 'K004'), + (8, 8, 'K004'), + ] + self.assert_has_errors(code, expected_errors=expected_errors) diff --git a/tox.ini b/tox.ini index cd7d59245c..d800b6817d 100644 --- a/tox.ini +++ b/tox.ini @@ -52,3 +52,4 @@ commands = {toxinidir}/tools/config/generate_sample.sh [hacking] import_exceptions = keystone.openstack.common.gettextutils._ +local-check-factory = keystone.hacking.checks.factory