From 1988587507cb2cb21efffac01ac9a8c3ebf291a5 Mon Sep 17 00:00:00 2001 From: Pierre Chevalier Date: Thu, 17 Oct 2019 15:09:11 +0100 Subject: [PATCH] Restore pycodestyle (#8471) ### Problem As part of the effort to migrate the codebase to black, pycodestyle was disabled under the assumption that the code would soon be formatted with black, making pycodestyle unnecessary. (See https://github.com/pantsbuild/pants/pull/8350/commits/bdd2b9ee0ea89028d7844ac34e957b3b0caedb87) Migrating to Black has taken longer than expected, so disabling pycodestyle has started to build up technical debt. ### Solution The reason pycodestyle formatting was a problem at the time was because unit tests for Black used existing files in our codebase, and some of those needed to be formatted consistently with how black would format them. Instead, craft temporary files on demand to test Black's functionality. Also, pay-off the technical debt that accumulated since disabling pycodestyle by fixing all linting errors that crept up. ### Result Python linting is back, the codebase lints cleanly and Black tests don't rely on indentation of python files in our codebase anymore. --- .../python/checks/checker/test_pyflakes.py | 4 - .../src/python/example/hello/greet/greet.py | 4 +- .../src/python/example/hello/main/main.py | 6 +- pants.ini | 3 - src/python/pants/backend/python/rules/pex.py | 10 +-- .../python/rules/python_create_binary.py | 1 - .../pants/backend/python/rules/python_fmt.py | 3 +- src/python/pants/engine/rules.py | 2 +- src/python/pants/rules/core/binary.py | 5 +- src/python/pants/scm/git.py | 2 +- src/python/pants/util/objects.py | 3 - .../ctypes_python_pkg/ctypes_wrapper.py | 1 + .../ctypes_python_pkg/ctypes_wrapper.py | 1 + .../ctypes_python_pkg/ctypes_wrapper.py | 1 + .../setup_requires/setup.py | 1 - .../setup_requires/test_setup_requires.py | 2 - .../rules/test_python_fmt_integration.py | 87 +++++++++++++------ .../python/pants_test/base/test_hash_utils.py | 1 + tests/python/pants_test/task_test_base.py | 2 +- tests/python/pants_test/util/test_objects.py | 1 + 20 files changed, 79 insertions(+), 61 deletions(-) diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_pyflakes.py b/contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_pyflakes.py index d1b65d38167..aaa3669a856 100644 --- a/contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_pyflakes.py +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_pyflakes.py @@ -25,10 +25,6 @@ def test_pyflakes_unused_import(self): # rather than with Pants. self.assertNit('import os', 'F401', expected_line_number='001-002') - def test_pyflakes_ignore(self): - plugin = self.get_plugin('import os', ignore=['F401']) - self.assertEqual([], list(plugin.nits())) - def test_pyflakes_ignore(self): plugin = self.get_plugin('import os', ignore=['UnusedImport']) self.assertEqual([], list(plugin.nits())) diff --git a/examples/src/python/example/hello/greet/greet.py b/examples/src/python/example/hello/greet/greet.py index 83207ad35a9..83f551e789d 100644 --- a/examples/src/python/example/hello/greet/greet.py +++ b/examples/src/python/example/hello/greet/greet.py @@ -5,5 +5,5 @@ def greet(greetee): - """Given the name, return a greeting for a person of that name.""" - return green("Hello, {}!".format(greetee)) + """Given the name, return a greeting for a person of that name.""" + return green("Hello, {}!".format(greetee)) diff --git a/examples/src/python/example/hello/main/main.py b/examples/src/python/example/hello/main/main.py index 7720ab31e38..1bad45da15e 100644 --- a/examples/src/python/example/hello/main/main.py +++ b/examples/src/python/example/hello/main/main.py @@ -7,6 +7,6 @@ if __name__ == "__main__": - greetees = sys.argv[1:] or ["world"] - for greetee in greetees: - print(greet(greetee)) + greetees = sys.argv[1:] or ["world"] + for greetee in greetees: + print(greet(greetee)) diff --git a/pants.ini b/pants.ini index 3c42243aa8b..df4d2475571 100644 --- a/pants.ini +++ b/pants.ini @@ -260,9 +260,6 @@ skip: True # --closure option is removed. See comment in the task code for details. transitive: True -[lint.pythonstyle] -skip: True - [lint.scalafmt] skip: True diff --git a/src/python/pants/backend/python/rules/pex.py b/src/python/pants/backend/python/rules/pex.py index b653535aa2c..2c7835a704d 100644 --- a/src/python/pants/backend/python/rules/pex.py +++ b/src/python/pants/backend/python/rules/pex.py @@ -15,16 +15,11 @@ DirectoriesToMerge, DirectoryWithPrefixToAdd, ) -from pants.engine.isolated_process import ( - ExecuteProcessRequest, - ExecuteProcessResult, - MultiPlatformExecuteProcessRequest, -) +from pants.engine.isolated_process import ExecuteProcessResult, MultiPlatformExecuteProcessRequest from pants.engine.legacy.structs import PythonTargetAdaptor, TargetAdaptor from pants.engine.platform import Platform, PlatformConstraint -from pants.engine.rules import RootRule, optionable_rule, rule +from pants.engine.rules import optionable_rule, rule from pants.engine.selectors import Get -from pants.util.strutil import create_path_env_var @dataclass(frozen=True) @@ -46,6 +41,7 @@ def create_from_adaptors(cls, adaptors: Tuple[TargetAdaptor, ...], additional_re all_target_requirements.extend(additional_requirements) return PexRequirements(requirements=tuple(sorted(all_target_requirements))) + @dataclass(frozen=True) class PexInterpreterContraints: constraint_set: FrozenSet[str] = frozenset() diff --git a/src/python/pants/backend/python/rules/python_create_binary.py b/src/python/pants/backend/python/rules/python_create_binary.py index bc5328a9a45..7ac741de07b 100644 --- a/src/python/pants/backend/python/rules/python_create_binary.py +++ b/src/python/pants/backend/python/rules/python_create_binary.py @@ -6,7 +6,6 @@ from pants.backend.python.subsystems.python_setup import PythonSetup from pants.backend.python.targets.python_binary import PythonBinary from pants.engine.fs import Digest, DirectoriesToMerge -from pants.engine.isolated_process import ExecuteProcessRequest, FallibleExecuteProcessResult from pants.engine.legacy.graph import BuildFileAddresses, HydratedTarget, TransitiveHydratedTargets from pants.engine.legacy.structs import PythonBinaryAdaptor from pants.engine.rules import UnionRule, rule diff --git a/src/python/pants/backend/python/rules/python_fmt.py b/src/python/pants/backend/python/rules/python_fmt.py index a2aa2514823..aae2fe6a724 100644 --- a/src/python/pants/backend/python/rules/python_fmt.py +++ b/src/python/pants/backend/python/rules/python_fmt.py @@ -29,7 +29,7 @@ # containing python files so we can have one single type used as an input in the run_black rule. @dataclass(frozen=True) class FormattablePythonTarget: - target: Any + target: Any @rule @@ -93,6 +93,7 @@ def run_black( stderr=result.stderr.decode(), ) + # TODO: remove this workaround once https://github.com/pantsbuild/pants/issues/8343 is addressed @rule def target_adaptor(target: PythonTargetAdaptor) -> FormattablePythonTarget: diff --git a/src/python/pants/engine/rules.py b/src/python/pants/engine/rules.py index 8f28171da29..7bf3f3777fb 100644 --- a/src/python/pants/engine/rules.py +++ b/src/python/pants/engine/rules.py @@ -511,7 +511,7 @@ def __str__(self): @property def output_type(self): - return self._output_type + return self._output_type @property def dependency_rules(self): diff --git a/src/python/pants/rules/core/binary.py b/src/python/pants/rules/core/binary.py index ecb876d8967..cf91a6475d1 100644 --- a/src/python/pants/rules/core/binary.py +++ b/src/python/pants/rules/core/binary.py @@ -2,17 +2,14 @@ # Licensed under the Apache License, Version 2.0 (see LICENSE). from dataclasses import dataclass -from typing import Any -from pants.backend.python.rules.pex import Pex from pants.build_graph.address import Address from pants.engine.addressable import BuildFileAddresses from pants.engine.console import Console from pants.engine.fs import Digest, DirectoryToMaterialize, Workspace from pants.engine.goal import Goal, LineOriented from pants.engine.legacy.graph import HydratedTarget -from pants.engine.legacy.structs import PythonBinaryAdaptor -from pants.engine.rules import console_rule, optionable_rule, rule, union +from pants.engine.rules import console_rule, rule, union from pants.engine.selectors import Get diff --git a/src/python/pants/scm/git.py b/src/python/pants/scm/git.py index 45b558b3059..095eb52d85c 100644 --- a/src/python/pants/scm/git.py +++ b/src/python/pants/scm/git.py @@ -12,7 +12,7 @@ from pants.scm.scm import Scm from pants.util.contextutil import pushd from pants.util.memo import memoized_method -from pants.util.strutil import ensure_binary, ensure_text +from pants.util.strutil import ensure_text # 40 is Linux's hard-coded limit for total symlinks followed when resolving a path. diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index 1f7e8763366..4ec980919d2 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -1,13 +1,10 @@ # Copyright 2016 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -import re from abc import ABC, abstractmethod from collections import OrderedDict, namedtuple from collections.abc import Iterable -from twitter.common.collections import OrderedSet - from pants.util.memo import memoized_classproperty from pants.util.meta import classproperty from pants.util.strutil import pluralize diff --git a/testprojects/src/python/python_distribution/ctypes/ctypes_python_pkg/ctypes_wrapper.py b/testprojects/src/python/python_distribution/ctypes/ctypes_python_pkg/ctypes_wrapper.py index ede26d6c943..dd50bb9a1f0 100644 --- a/testprojects/src/python/python_distribution/ctypes/ctypes_python_pkg/ctypes_wrapper.py +++ b/testprojects/src/python/python_distribution/ctypes/ctypes_python_pkg/ctypes_wrapper.py @@ -19,6 +19,7 @@ def get_generated_shared_lib(lib_name): asdf_c_lib = ctypes.CDLL(asdf_c_lib_path) asdf_cpp_lib = ctypes.CDLL(asdf_cpp_lib_path) + def f(x): added = asdf_c_lib.add_three(x) multiplied = asdf_cpp_lib.multiply_by_three(added) diff --git a/testprojects/src/python/python_distribution/ctypes_with_extra_compiler_flags/ctypes_python_pkg/ctypes_wrapper.py b/testprojects/src/python/python_distribution/ctypes_with_extra_compiler_flags/ctypes_python_pkg/ctypes_wrapper.py index ea04dd7e2d9..d44acec2b35 100644 --- a/testprojects/src/python/python_distribution/ctypes_with_extra_compiler_flags/ctypes_python_pkg/ctypes_wrapper.py +++ b/testprojects/src/python/python_distribution/ctypes_with_extra_compiler_flags/ctypes_python_pkg/ctypes_wrapper.py @@ -16,6 +16,7 @@ def get_generated_shared_lib(lib_name): asdf_cpp_lib_path = get_generated_shared_lib('asdf-cpp_ctypes-with-extra-compiler-flags') asdf_cpp_lib = ctypes.CDLL(asdf_cpp_lib_path) + def f(x): multiplied = asdf_cpp_lib.multiply_by_something(42) return multiplied diff --git a/testprojects/src/python/python_distribution/ctypes_with_third_party/ctypes_python_pkg/ctypes_wrapper.py b/testprojects/src/python/python_distribution/ctypes_with_third_party/ctypes_python_pkg/ctypes_wrapper.py index cb34d9eabf8..26e6221b587 100644 --- a/testprojects/src/python/python_distribution/ctypes_with_third_party/ctypes_python_pkg/ctypes_wrapper.py +++ b/testprojects/src/python/python_distribution/ctypes_with_third_party/ctypes_python_pkg/ctypes_wrapper.py @@ -16,6 +16,7 @@ def get_generated_shared_lib(lib_name): asdf_cpp_lib_path = get_generated_shared_lib('asdf-cpp_ctypes-with-third-party') asdf_cpp_lib = ctypes.CDLL(asdf_cpp_lib_path) + def f(x): added = x + 3 multiplied = asdf_cpp_lib.multiply_by_three(added) diff --git a/testprojects/src/python/python_distribution/setup_requires/setup.py b/testprojects/src/python/python_distribution/setup_requires/setup.py index d201f37f754..8ee1e8becb7 100644 --- a/testprojects/src/python/python_distribution/setup_requires/setup.py +++ b/testprojects/src/python/python_distribution/setup_requires/setup.py @@ -1,7 +1,6 @@ # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -import os from pathlib import Path from setuptools import find_packages, setup diff --git a/testprojects/src/python/python_distribution/setup_requires/test_setup_requires.py b/testprojects/src/python/python_distribution/setup_requires/test_setup_requires.py index a27b435b85d..2e4812585fd 100644 --- a/testprojects/src/python/python_distribution/setup_requires/test_setup_requires.py +++ b/testprojects/src/python/python_distribution/setup_requires/test_setup_requires.py @@ -1,8 +1,6 @@ # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -import re - from checksum import checksum diff --git a/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py b/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py index de34b39b0c3..ca9fef6292b 100644 --- a/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py +++ b/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py @@ -1,36 +1,74 @@ # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). +import os +from contextlib import contextmanager from os.path import relpath from pathlib import Path -from pants.util.contextutil import temporary_file, temporary_file_path -from pants_test.pants_run_integration_test import PantsRunIntegrationTest, ensure_daemon +from pants.util.contextutil import temporary_dir, temporary_file_path +from pants.util.dirutil import safe_file_dump +from pants_test.pants_run_integration_test import PantsRunIntegrationTest + + +def write_build_file(root_dir): + safe_file_dump(os.path.join(root_dir, "BUILD"), "python_library()") + +INCONSISTENTLY_FORMATTED_HELLO: str = "def hello():x=42" +CONSISTENTLY_FORMATTED_HELLO: str = "def hello():\n x = 42\n" + + +def write_inconsistently_formatted_file(root_dir, filename) -> Path: + filepath = os.path.join(root_dir, filename) + safe_file_dump(filepath, INCONSISTENTLY_FORMATTED_HELLO) + return Path(filepath) + + +def write_consistently_formatted_file(root_dir, filename) -> Path: + filepath = os.path.join(root_dir, filename) + safe_file_dump(filepath, CONSISTENTLY_FORMATTED_HELLO) + return Path(filepath) + + +@contextmanager +def build_directory(): + with temporary_dir(root_dir=".") as root_dir: + write_build_file(root_dir) + yield root_dir class PythonFmtIntegrationTest(PantsRunIntegrationTest): def test_black_one_python_source_should_leave_one_file_unchanged(self): - command = [ - 'fmt-v2', - 'examples/src/python/example/hello/main:main' + with build_directory() as root_dir: + code = write_consistently_formatted_file(root_dir, "hello.py") + command = [ + 'fmt-v2', + f'{root_dir}' ] - pants_run = self.run_pants(command=command) - self.assert_success(pants_run) + pants_run = self.run_pants(command=command) + self.assert_success(pants_run) + after_formatting = code.read_text() + self.assertEqual(CONSISTENTLY_FORMATTED_HELLO, after_formatting) self.assertNotIn("reformatted", pants_run.stderr_data) self.assertIn("1 file left unchanged", pants_run.stderr_data) - def test_black_two_python_sources_should_leave_two_files_unchanged(self): - command = [ - 'fmt-v2', - 'examples/src/python/example/hello/greet:greet' + with build_directory() as root_dir: + foo = write_consistently_formatted_file(root_dir, "foo.py") + bar = write_consistently_formatted_file(root_dir, "bar.py") + command = [ + 'fmt-v2', + f'{root_dir}' ] - pants_run = self.run_pants(command=command) - self.assert_success(pants_run) + pants_run = self.run_pants(command=command) + self.assert_success(pants_run) + foo_after_formatting = foo.read_text() + self.assertEqual(CONSISTENTLY_FORMATTED_HELLO, foo_after_formatting) + bar_after_formatting = bar.read_text() + self.assertEqual(CONSISTENTLY_FORMATTED_HELLO, bar_after_formatting) self.assertNotIn("reformatted", pants_run.stderr_data) self.assertIn("2 files left unchanged", pants_run.stderr_data) - def test_black_should_pickup_default_config(self): # If the default config (pyproject.toml is picked up, the compilation_failure target will be skipped command = [ @@ -43,7 +81,6 @@ def test_black_should_pickup_default_config(self): self.assertNotIn("unchanged", pants_run.stderr_data) self.assertIn("Nothing to do", pants_run.stderr_data) - def test_black_should_pickup_non_default_config(self): # If a valid toml file without a black configuration section is picked up, # Black won't skip the compilation_failure and will fail @@ -59,20 +96,16 @@ def test_black_should_pickup_non_default_config(self): self.assertNotIn("unchanged", pants_run.stderr_data) self.assertIn("1 file failed to reformat", pants_run.stderr_data) - def test_black_should_format_python_code(self): - # Open file in the greet target as the BUILD file globs python files from there - with temporary_file(root_dir="./examples/src/python/example/hello/greet/", suffix=".py") as code: - file_name = code.name - code.write(b"x = 42") - code.close() + with build_directory() as root_dir: + code = write_inconsistently_formatted_file(root_dir, "hello.py") command = [ 'fmt-v2', - 'examples/src/python/example/hello/greet:greet' - ] + f'{root_dir}' + ] pants_run = self.run_pants(command=command) - formatted = Path(file_name).read_text(); - self.assertEqual("x = 42\n", formatted) - self.assert_success(pants_run) + self.assert_success(pants_run) + after_formatting = code.read_text() + self.assertEqual(CONSISTENTLY_FORMATTED_HELLO, after_formatting) self.assertIn("1 file reformatted", pants_run.stderr_data) - self.assertIn("2 files left unchanged", pants_run.stderr_data) + self.assertNotIn("unchanged", pants_run.stderr_data) diff --git a/tests/python/pants_test/base/test_hash_utils.py b/tests/python/pants_test/base/test_hash_utils.py index 16c30428a5f..168ba6e6acd 100644 --- a/tests/python/pants_test/base/test_hash_utils.py +++ b/tests/python/pants_test/base/test_hash_utils.py @@ -187,6 +187,7 @@ class Test(Enum): pig = 2 self.assertEqual(self._coercing_json_encode([Test.dog, Test.cat, Test.pig]), '[0, 1, 2]') + class JsonHashingTest(unittest.TestCase): def test_known_checksums(self): diff --git a/tests/python/pants_test/task_test_base.py b/tests/python/pants_test/task_test_base.py index ca2c882b8c8..79da5267d5e 100644 --- a/tests/python/pants_test/task_test_base.py +++ b/tests/python/pants_test/task_test_base.py @@ -335,7 +335,7 @@ class TaskInvocationResult: this_task: Task after_tasks: Tuple[Task, ...] - def invoke_tasks(self, target_closure=None, **context_kwargs) -> TaskInvocationResult: + def invoke_tasks(self, target_closure=None, **context_kwargs) -> 'TaskInvocationResult': """Create and execute the declaratively specified tasks in order. Create instances of and execute task types in `self.run_before_task_types()`, then diff --git a/tests/python/pants_test/util/test_objects.py b/tests/python/pants_test/util/test_objects.py index 219a2086c1c..c9130ca21c5 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -323,6 +323,7 @@ class ReturnsNotImplemented: def __eq__(self, other): return NotImplemented + class DatatypeTest(TestBase): def test_eq_with_not_implemented_super(self):