Add try/except/finally complexity rules

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

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,13 +46,37 @@ 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 = (
         ('ignore-module-import-only', {
             '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})

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

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)