From 25798d6d0a69e9a3c0164d2fd1d091dfb5a0a39a Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 17:39:15 -0500 Subject: [PATCH 1/3] Add try/except/finally complexity rules --- shopify_python/ast.py | 8 ++++ shopify_python/google_styleguide.py | 47 +++++++++++++++++++ tests/shopify_python/test_ast.py | 11 +++++ .../shopify_python/test_google_styleguide.py | 25 +++++++++- 4 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 shopify_python/ast.py create mode 100644 tests/shopify_python/test_ast.py 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..cf7c636 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': 20, + 'type': 'int', + 'help': 'Number of AST nodes permitted in a try-block'}), + ('max-except-nodes', { + 'default': 30, + 'type': 'int', + 'help': 'Number of AST nodes permitted in an except-block'}), + ('max-finally-nodes', { + 'default': 10, + '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) @@ -129,3 +160,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..70d4e4a 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,26 @@ 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)) + 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)) + """) + with self.assertAddsMessages( + pylint.testutils.Message('finally-too-long', node=root.body[0], args={'found': 24}), + pylint.testutils.Message('try-too-long', node=root.body[0].body[0], args={'found': 24}), + pylint.testutils.Message('except-too-long', node=root.body[0].body[0].handlers[0], + args={'found': 39}), + ): + self.walk(root) From 1430c96abc6d0a1708ad2488827ce37925f00001 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Tue, 7 Mar 2017 22:06:56 -0500 Subject: [PATCH 2/3] Set default max sizes using 80th percentile of large codebase --- shopify_python/google_styleguide.py | 9 +++++---- tests/shopify_python/test_google_styleguide.py | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index cf7c636..22c1f6f 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -66,15 +66,15 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): 'type': 'csv', 'help': 'List of top-level module names separated by comma.'}), ('max-try-nodes', { - 'default': 20, + 'default': 25, 'type': 'int', 'help': 'Number of AST nodes permitted in a try-block'}), ('max-except-nodes', { - 'default': 30, + 'default': 23, 'type': 'int', 'help': 'Number of AST nodes permitted in an except-block'}), ('max-finally-nodes', { - 'default': 10, + 'default': 13, 'type': 'int', 'help': 'Number of AST nodes permitted in a finally-block'}), ) @@ -116,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 diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index 70d4e4a..ac2ba40 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -118,7 +118,7 @@ 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 = 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]) @@ -131,7 +131,7 @@ def test_try_exc_fnly_complexity(self): """) with self.assertAddsMessages( pylint.testutils.Message('finally-too-long', node=root.body[0], args={'found': 24}), - pylint.testutils.Message('try-too-long', node=root.body[0].body[0], args={'found': 24}), + pylint.testutils.Message('try-too-long', node=root.body[0].body[0], args={'found': 28}), pylint.testutils.Message('except-too-long', node=root.body[0].body[0].handlers[0], args={'found': 39}), ): From a25a373b8da99f31eaf3a52e8f97844fc6e3e070 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Wed, 8 Mar 2017 15:51:14 -0500 Subject: [PATCH 3/3] Give nodes descriptive names in tests --- tests/shopify_python/test_google_styleguide.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index ac2ba40..4fdc000 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -129,10 +129,11 @@ def test_try_exc_fnly_complexity(self): 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=root.body[0], args={'found': 24}), - pylint.testutils.Message('try-too-long', node=root.body[0].body[0], args={'found': 28}), - pylint.testutils.Message('except-too-long', node=root.body[0].body[0].handlers[0], - args={'found': 39}), + 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)