diff --git a/shopify_python/ast.py b/shopify_python/ast.py new file mode 100644 index 0000000..ea8f516 --- /dev/null +++ b/shopify_python/ast.py @@ -0,0 +1,8 @@ +import astroid # pylint: disable=unused-import + + +def count_tree_size(node): # type: (astroid.NodeNG) -> int + size = 1 + for child in node.get_children(): + size += count_tree_size(child) + return size diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index af8f5c5..22c1f6f 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -1,6 +1,7 @@ import typing # pylint: disable=unused-import import astroid # pylint: disable=unused-import +import shopify_python.ast import six from pylint import checkers @@ -45,6 +46,18 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): 'C2606': ('Caught StandardError', 'catch-standard-error', "Don't catch StandardError"), + 'C2607': ('Try body has %(found)i nodes', + 'try-too-long', + "%(found)i nodes is a larger than recommended 'try' body size making it more " + "likely that an unexpected exception will be raised"), + 'C2608': ('Except body has %(found)i nodes', + 'except-too-long', + "%(found)i nodes is a larger than recommended 'except' body size making it more " + "likely that an exception will be raised during exception handling"), + 'C2609': ('Finally body has %(found)i nodes', + 'finally-too-long', + "%(found)i nodes is a larger than recommended 'finally' body size making it more " + "likely that an exception will be raised during resource cleanup activities"), } options = ( @@ -52,6 +65,18 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): 'default': ('__future__',), 'type': 'csv', 'help': 'List of top-level module names separated by comma.'}), + ('max-try-nodes', { + 'default': 25, + 'type': 'int', + 'help': 'Number of AST nodes permitted in a try-block'}), + ('max-except-nodes', { + 'default': 23, + 'type': 'int', + 'help': 'Number of AST nodes permitted in an except-block'}), + ('max-finally-nodes', { + 'default': 13, + 'type': 'int', + 'help': 'Number of AST nodes permitted in a finally-block'}), ) def visit_assign(self, node): # type: (astroid.Assign) -> None @@ -60,6 +85,12 @@ def visit_assign(self, node): # type: (astroid.Assign) -> None def visit_excepthandler(self, node): # type: (astroid.ExceptHandler) -> None self.__dont_catch_standard_error(node) + def visit_tryexcept(self, node): # type: (astroid.TryExcept) -> None + self.__minimize_code_in_try_except(node) + + def visit_tryfinally(self, node): # type: (astroid.TryFinally) -> None + self.__minimize_code_in_finally(node) + def visit_importfrom(self, node): # type: (astroid.ImportFrom) -> None self.__import_modules_only(node) self.__import_full_path_only(node) @@ -85,11 +116,12 @@ def __import_modules_only(self, node): # type: (astroid.ImportFrom) -> None # Warn on each imported name (yi) in "from x import y1, y2, y3" for child_module in self.__get_module_names(node): + args = {'child': child_module} try: parent.import_module(child_module) except astroid.exceptions.AstroidBuildingException as building_exception: if str(building_exception).startswith('Unable to load module'): - self.add_message('import-modules-only', node=node, args={'child': child_module}) + self.add_message('import-modules-only', node=node, args=args) else: raise @@ -129,3 +161,19 @@ def __dont_catch_standard_error(self, node): # type: (astroid.ExceptHandler) -> """ if hasattr(node.type, 'name') and node.type.name == 'StandardError': self.add_message('catch-standard-error', node=node) + + def __minimize_code_in_try_except(self, node): # type: (astroid.TryExcept) -> None + """Minimize the amount of code in a try/except block.""" + try_body_nodes = sum((shopify_python.ast.count_tree_size(child) for child in node.body)) + if try_body_nodes > self.config.max_try_nodes: # pylint: disable=no-member + self.add_message('try-too-long', node=node, args={'found': try_body_nodes}) + for handler in node.handlers: + except_nodes = shopify_python.ast.count_tree_size(handler) + if except_nodes > self.config.max_except_nodes: # pylint: disable=no-member + self.add_message('except-too-long', node=handler, args={'found': except_nodes}) + + def __minimize_code_in_finally(self, node): # type: (astroid.TryFinally) -> None + """Minimize the amount of code in a finally block.""" + finally_body_nodes = sum((shopify_python.ast.count_tree_size(child) for child in node.finalbody)) + if finally_body_nodes > self.config.max_finally_nodes: # pylint: disable=no-member + self.add_message('finally-too-long', node=node, args={'found': finally_body_nodes}) diff --git a/tests/shopify_python/test_ast.py b/tests/shopify_python/test_ast.py new file mode 100644 index 0000000..379c887 --- /dev/null +++ b/tests/shopify_python/test_ast.py @@ -0,0 +1,11 @@ +import astroid + +from shopify_python import ast + + +def test_count_tree_size(): + root = astroid.builder.parse(""" + def test(x, y): + return x * y if x > 5 else 0 + """) + assert ast.count_tree_size(root) == 14 diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index 70a31c5..4fdc000 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -1,6 +1,6 @@ import sys -import astroid.test_utils +import astroid import pylint.testutils import pytest @@ -113,3 +113,27 @@ def test_catch_blank_passes(self): """) with self.assertAddsMessages(): self.walk(root) + + def test_try_exc_fnly_complexity(self): + root = astroid.builder.parse(""" + try: + a = [x for x in range(0, 10) if x < 5] + a = sum((x * 2 for x in a)) + (a ** 2) + except AssertionError as assert_error: + if set(assert_error).startswith('Division by zero'): + raise MyException(assert_error, [x for x in range(0, 10) if x < 5]) + else: + a = [x for x in range(0, 10) if x < 5] + raise + finally: + a = [x for x in range(0, 10) if x < 5] + a = sum((x * 2 for x in a)) + """) + try_finally = root.body[0] + try_except = try_finally.body[0] + with self.assertAddsMessages( + pylint.testutils.Message('finally-too-long', node=try_finally, args={'found': 24}), + pylint.testutils.Message('try-too-long', node=try_except, args={'found': 28}), + pylint.testutils.Message('except-too-long', node=try_except.handlers[0], args={'found': 39}), + ): + self.walk(root)