Use simple lambdas

shopify_python/git_utils.py

@@ -7,19 +7,22 @@
 class GitUtilsException(Exception):
     pass
 
+
 def _remote_origin_master(git_repo):
     # type: (repo.Repo) -> head.Head
     remote_master = git_repo.heads.master.tracking_branch()
     if not remote_master or not remote_master.is_valid():
         raise GitUtilsException("Unable to locate remote branch origin/master")
     return remote_master
 
+
 def _modified_in_branch(git_repo, other_ref):
     # type: (repo.Repo, head.Head) -> typing.List[str]
     commit = git_repo.active_branch.commit
     modified = [x for x in commit.diff(other_ref.commit, R=True) if not x.deleted_file]
     return [x.b_path for x in modified]
 
+
 def _file_is_python(path):
     # type: (str) -> bool
     if path.endswith('.py'):
@@ -35,6 +38,7 @@ def _file_is_python(path):
                 pass
         return False
 
+
 def changed_python_files_in_tree(root_path):
     # type: (str) -> typing.List[str]
 

shopify_python/google_styleguide.py

@@ -66,7 +66,11 @@ class GoogleStyleGuideChecker(checkers.BaseChecker):
         'C6010': ('Statement imports multiple items from %(module)s',
                   'multiple-import-items',
                   'Multiple imports usually result in noisy and potentially conflicting git diffs. To alleviate, '
-                  'separate imports into one item per line.')
+                  'separate imports into one item per line.'),
+        'C6011': ('Lambda has %(found)i nodes',
+                  'lambda-too-long',
+                  "Okay to use them for one-liners. If the code inside the lambda function is any longer than a "
+                  "certain length, it's probably better to define it as a regular (nested) function."),
     }
 
     options = (
@@ -86,6 +90,10 @@ class GoogleStyleGuideChecker(checkers.BaseChecker):
             'default': 13,
             'type': 'int',
             'help': 'Number of AST nodes permitted in a finally-block'}),
+        ('max-lambda-nodes', {
+            'default': 15,
+            'type': 'int',
+            'help': 'Number of AST nodes permitted in a lambda'}),
     )
 
     def visit_assign(self, node):  # type: (astroid.Assign) -> None
@@ -94,6 +102,9 @@ 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_lambda(self, node):  # type: (astroid.Lambda) -> None
+        self.__use_simple_lambdas(node)
+
     def visit_tryexcept(self, node):  # type: (astroid.TryExcept) -> None
         self.__minimize_code_in_try_except(node)
 
@@ -201,3 +212,7 @@ def __minimize_code_in_finally(self, node):  # type: (astroid.TryFinally) -> Non
         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})
+
+    def __use_simple_lambdas(self, node):  # type: (astroid.Lambda) -> None
+        if shopify_python.ast.count_tree_size(node) > self.config.max_lambda_nodes:  # pylint: disable=no-member
+            self.add_message('use-simple-lambdas', node=node)

tests/shopify_python/test_git_utils.py

@@ -4,6 +4,7 @@
 from git import repo
 from shopify_python import git_utils
 
+
 @pytest.fixture
 def python_file(main_repo):
     # type: (repo.Repo) -> str
@@ -18,6 +19,7 @@ def python_file(main_repo):
         writing_file.writelines(file_lines)
     return file_path
 
+
 @pytest.fixture
 def python_script(main_repo):
     # type: (repo.Repo) -> str
@@ -33,6 +35,7 @@ def python_script(main_repo):
         writing_file.writelines(file_lines)
     return file_path
 
+
 @pytest.fixture
 def non_python_file(main_repo):
     # type: (repo.Repo) -> str
@@ -47,11 +50,13 @@ def non_python_file(main_repo):
         writing_file.writelines(file_lines)
     return file_path
 
+
 @pytest.fixture
 def remote_repo(tmpdir):
     # type: ('py.path.LocalPath') -> repo.Repo
     return repo.Repo.init(str(tmpdir.join('remote')), bare=True)
 
+
 @pytest.fixture
 def main_repo(tmpdir, remote_repo):
     # type: ('py.path.LocalPath', repo.Repo) -> repo.Repo
@@ -66,6 +71,7 @@ def main_repo(tmpdir, remote_repo):
 
     return new_repo
 
+
 def test_detects_changed_python_files(main_repo, python_file, python_script):
     # type: (repo.Repo, str, str) -> None
 
@@ -79,6 +85,7 @@ def test_detects_changed_python_files(main_repo, python_file, python_script):
         os.path.basename(python_file),
     ]
 
+
 def test_doesnt_include_changed_nonpython_files(main_repo, python_file, non_python_file):
     # type: (repo.Repo, str, str) -> None
 
@@ -89,6 +96,7 @@ def test_doesnt_include_changed_nonpython_files(main_repo, python_file, non_pyth
     changed_files = git_utils.changed_python_files_in_tree(main_repo.working_dir)
     assert changed_files == [os.path.basename(python_file)]
 
+
 def test_only_include_modified_locally(main_repo, python_file):
     # type: (repo.Repo, str) -> None
 
@@ -118,6 +126,7 @@ def test_only_include_modified_locally(main_repo, python_file):
     # only the one new file is added
     assert git_utils.changed_python_files_in_tree(main_repo.working_dir) == [os.path.basename(python_file)]
 
+
 def test_cant_find_remote_origin(main_repo, remote_repo):
     # type: (repo.Repo, repo.Repo) -> None
     main_repo.create_remote('foo', remote_repo.working_dir)
@@ -127,6 +136,7 @@ def test_cant_find_remote_origin(main_repo, remote_repo):
         git_utils.changed_python_files_in_tree(main_repo.working_dir)
     assert "Unable to locate remote branch origin/master" in ex.exconly()
 
+
 def test_cant_find_origin_master(main_repo, remote_repo):
     # type: (repo.Repo, repo.Repo) -> None
 
@@ -140,6 +150,7 @@ def test_cant_find_origin_master(main_repo, remote_repo):
         git_utils.changed_python_files_in_tree(main_repo.working_dir)
     assert "Unable to locate remote branch origin/master" in ex.exconly()
 
+
 def test_dont_include_deleted_files(main_repo, python_file):
     # type: (repo.Repo, str) -> None
 
@@ -154,6 +165,7 @@ def test_dont_include_deleted_files(main_repo, python_file):
 
     assert git_utils.changed_python_files_in_tree(main_repo.working_dir) == []
 
+
 def test_include_modified_files(main_repo, python_file):
     # type: (repo.Repo, str) -> None
 
@@ -170,6 +182,7 @@ def test_include_modified_files(main_repo, python_file):
     main_repo.index.commit("modifying python file")
     assert git_utils.changed_python_files_in_tree(main_repo.working_dir) == [os.path.basename(python_file)]
 
+
 def test_dont_include_uncommited_or_untracked_files(main_repo, python_file):
     # type: (repo.Repo, str) -> None
 
@@ -180,6 +193,7 @@ def test_dont_include_uncommited_or_untracked_files(main_repo, python_file):
     main_repo.index.commit("adding python file")
     assert git_utils.changed_python_files_in_tree(main_repo.working_dir) == [os.path.basename(python_file)]
 
+
 def test_dont_include_scripts_with_extensions(main_repo):
     # type: (repo.Repo) -> None
 
@@ -195,6 +209,7 @@ def test_dont_include_scripts_with_extensions(main_repo):
     main_repo.index.commit("adding script file with .sh extension")
     assert git_utils.changed_python_files_in_tree(main_repo.working_dir) == []
 
+
 def test_dont_include_binary_files(main_repo):
     # type: (repo.Repo) -> None
 

tests/shopify_python/test_google_styleguide.py

@@ -167,6 +167,22 @@ def test_multiple_from_imports(self):
         """)
 
         module2_node = root['module2']
-        message = pylint.testutils.Message('multiple-import-items', node=module2_node, args={'module': 'package.module'})
+        message = pylint.testutils.Message(
+            'multiple-import-items',
+            node=module2_node,
+            args={
+                'module': 'package.module'})
         with self.assert_adds_code_messages(['multiple-import-items'], message):
             self.walk(root)
+
+    def test_use_simple_lamndas(self):
+        root = astroid.builder.parse("""
+        def fnc():
+            good = lambda x, y: x if x % 2 == 0 else y
+            bad = lambda x, y: (x * 2 * 3 + 4) if x % 2 == 0 else (y * 2 * 3 + 4)
+        """)
+        fnc = root.body[0]
+        bad_list_comp = fnc.body[1].value
+        message = pylint.testutils.Message('use-simple-lambdas', node=bad_list_comp)
+        with self.assertAddsMessages(message):
+            self.walk(root)