openstack/keystone
Code Issues Proposed changes

Adds style checks to ease reviewer burden

Browse Source 
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
This commit is contained in:
David Stanek 2014-03-05 07:04:41 +00:00
parent da4d4a1035
commit ec6d5d4718
5 changed files with 278 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

0
keystone/hacking/__init__.py Normal file
View File

141
keystone/hacking/checks.py Normal file
View File

@ -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)

6
keystone/tests/test_backend.py
View File

@ -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

133
keystone/tests/test_hacking_checks.py Normal file
View File

@ -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)

1
tox.ini
View File

@ -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