diff --git a/shopify_python/git_utils.py b/shopify_python/git_utils.py index d37c717..81234f3 100644 --- a/shopify_python/git_utils.py +++ b/shopify_python/git_utils.py @@ -7,6 +7,7 @@ class GitUtilsException(Exception): pass + def _remote_origin_master(git_repo): # type: (repo.Repo) -> head.Head remote_master = git_repo.heads.master.tracking_branch() @@ -14,12 +15,14 @@ def _remote_origin_master(git_repo): 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] diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index 96b47f6..b3a1b42 100644 --- a/shopify_python/google_styleguide.py +++ b/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) diff --git a/tests/shopify_python/test_git_utils.py b/tests/shopify_python/test_git_utils.py index 68a141c..a65a35e 100644 --- a/tests/shopify_python/test_git_utils.py +++ b/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 diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index 9788287..da1226d 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/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)