Skip to content

Commit

Permalink
Remove pants.contrib.mypy plugin in favor of v2 implementation (#10157
Browse files Browse the repository at this point in the history
)

Some benefits:
* Runs in parallel with other linters.
* We can now delete v1 MyPy.
* We don't need the `type_checked` tag anymore. It's safe to run `./pants lint ::`. This means no more `mypy.py` script!

This implementation is not perfect (#10131), especially with performance.

[ci skip-rust-tests]
[ci skip-jvm-tests]
  • Loading branch information
Eric-Arellano authored Jun 26, 2020
1 parent 5f8a4ab commit c483495
Show file tree
Hide file tree
Showing 48 changed files with 109 additions and 922 deletions.
1 change: 0 additions & 1 deletion 3rdparty/python/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ fasteners==0.15.0
# for the mypy task since it configures custom MyPy plugins. That requirement can be found via:
#
# ./pants \
# --backend-packages=pants.contrib.mypy \
# options \
# --output-format=json \
# --scope=mypy \
Expand Down
9 changes: 0 additions & 9 deletions build-support/bin/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,6 @@ python_binary(
tags = {'type_checked'},
)

python_binary(
name = 'mypy',
sources = ['mypy.py'],
dependencies = [
':common',
],
tags = {'type_checked'},
)

python_binary(
name = 'shellcheck',
sources = ['shellcheck.py'],
Expand Down
17 changes: 5 additions & 12 deletions build-support/bin/ci.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,24 +430,17 @@ def run_check(command: List[str]) -> None:


def run_lint(*, oauth_token_path: Optional[str] = None) -> None:
targets = ["contrib::", "examples::", "src::", "tests::", "zinc::"]
targets = ["build-support::", "contrib::", "examples::", "src::", "tests::"]
command_prefix = ["./pants.pex", "--tag=-nolint"]

v2_command = (
command = (
[*command_prefix, "--no-v1", "--v2", "lint", *targets]
if oauth_token_path is None
else [*command_prefix, *_use_remote_execution(oauth_token_path), "lint", *targets]
)
_run_command(
v2_command,
slug="Lint (V2)",
start_message="Running V2 lint checks",
die_message="Lint check failure.",
)
_run_command(
[*command_prefix, "lint", *targets],
slug="Lint (V1)",
start_message="Running V1 lint checks",
command,
slug="Lint",
start_message="Running lint checks",
die_message="Lint check failure.",
)

Expand Down
53 changes: 0 additions & 53 deletions build-support/bin/mypy.py

This file was deleted.

6 changes: 1 addition & 5 deletions build-support/bin/packages.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,7 @@ def core_packages() -> Set[Package]:


def contrib_packages() -> Set[Package]:
return {
Package(
"pantsbuild.pants.contrib.mypy", "contrib/mypy/src/python/pants/contrib/mypy:plugin",
),
}
return set()


def all_packages() -> Set[Package]:
Expand Down
5 changes: 1 addition & 4 deletions build-support/githooks/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,7 @@ if git rev-parse --verify "${MERGE_BASE}" &>/dev/null; then
build-support/bin/check_rust_target_headers.sh || exit 1
fi

echo "* Checking types"
./build-support/bin/mypy.py || exit 1

echo "* Checking formatting and Flake8"
echo "* Checking MyPy, Flake8, and formatting"
./v2 --changed-since="${MERGE_BASE}" --tag='-nolint' lint || \
die "If there were formatting errors, run \`./v2 --changed-since=$(git rev-parse --symbolic "${MERGE_BASE}") fmt\`"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def test_update_build_file() -> None:
python_library(
sources=['good.py'],
)
python_tests(
{}
)
Expand All @@ -90,4 +90,4 @@ def test_update_build_file() -> None:
build = Path(tmpdir, "BUILD")
build.write_text(template.format("source='bad.py'"))
rewritten = maybe_rewrite_build(build)
assert "\n".join(rewritten) + "\n" == template.format("sources=['bad.py']")
assert "\n".join(rewritten or ()) + "\n" == template.format("sources=['bad.py']")
20 changes: 10 additions & 10 deletions build-support/migration-support/fix_deprecated_globs_usage.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,11 @@ def parse(cls, glob_func: ast.Call, *, build_file: Path) -> Optional["GlobFuncti
# NB: technically, glob arguments can be different than `globs`, `rglobs`, and `zglobs`, such
# as using `set()` in an `exclude` clause. We don't try to handle this edge case.
try:
glob_type = GlobType(glob_func.func.id)
glob_type = GlobType(glob_func.func.id) # type: ignore[attr-defined]
except ValueError:
logging.warning(
f"Could not parse the glob type `{glob_func.func.id}` in {build_file} at line "
f"{glob_func.lineno}. Please manually update."
f"Could not parse the glob type `{glob_func.func.id}` in {build_file} at " # type: ignore[attr-defined]
f"line {glob_func.lineno}. Please manually update."
)
return None
if not all(isinstance(arg, ast.Str) for arg in glob_func.args):
Expand All @@ -118,7 +118,7 @@ def parse(cls, glob_func: ast.Call, *, build_file: Path) -> Optional["GlobFuncti
f"using variables instead of raw strings. Please manually update."
)
return None
include_globs: List[str] = [arg.s for arg in glob_func.args]
include_globs: List[str] = [arg.s for arg in glob_func.args] # type: ignore[attr-defined]

# Excludes are tricky...The optional `exclude` keyword is guaranteed to have a list as its
# value, but that list can have any of these elements:
Expand All @@ -128,10 +128,10 @@ def parse(cls, glob_func: ast.Call, *, build_file: Path) -> Optional["GlobFuncti
exclude_globs: Optional[List[str]] = None
exclude_arg: Optional[ast.keyword] = next(iter(glob_func.keywords), None)
if exclude_arg is not None and isinstance(exclude_arg.value, ast.List):
exclude_elements: List[Union[ast.Call, ast.Str, ast.List]] = exclude_arg.value.elts
exclude_elements: List[Union[ast.Call, ast.Str, ast.List]] = exclude_arg.value.elts # type: ignore[assignment]
nested_exclude_elements: List[Union[ast.Call, ast.Str]] = list(
itertools.chain.from_iterable(
nested_list.elts
nested_list.elts # type: ignore[misc]
for nested_list in exclude_elements
if isinstance(nested_list, ast.List)
)
Expand Down Expand Up @@ -225,12 +225,12 @@ def generate_possibly_new_build(build_file: Path) -> Optional[List[str]]:
if bundles_arg is not None:
bundle_funcs: List[ast.Call] = [
element
for element in bundles_arg.value.elts
if isinstance(element, ast.Call) and element.func.id == "bundle"
for element in bundles_arg.value.elts # type: ignore[attr-defined]
if isinstance(element, ast.Call) and element.func.id == "bundle" # type: ignore[attr-defined]
]
for bundle_func in bundle_funcs:
# Every `bundle` is guaranteed to have a `fileset` defined.
fileset_arg: [ast.keyword] = next(
fileset_arg: [ast.keyword] = next( # type: ignore[misc]
kwarg for kwarg in bundle_func.keywords if kwarg.arg == "fileset"
)
if not isinstance(fileset_arg.value, ast.Call):
Expand Down Expand Up @@ -264,7 +264,7 @@ def generate_possibly_new_build(build_file: Path) -> Optional[List[str]]:
if parsed_glob_function is None:
continue

lineno: int = sources_arg.value.lineno
lineno: int = sources_arg.value.lineno # type: ignore[no-redef]
original_line = updated_text_lines[lineno - 1].rstrip()
formatted_replacement = parsed_glob_function.convert_to_sources_list(
use_single_quotes=use_single_quotes(original_line),
Expand Down
14 changes: 7 additions & 7 deletions build-support/migration-support/migrate_to_toml_config_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def test_fully_automatable_config() -> None:
bool_opt2: True
int_opt: 10
float_opt: 5.0
[str_values]
normal: .isort.cfg
version: isort>=4.8
Expand All @@ -45,7 +45,7 @@ def test_fully_automatable_config() -> None:
bool_opt2 = true
int_opt = 10
float_opt = 5.0
[str_values]
normal = ".isort.cfg"
version = "isort>=4.8"
Expand Down Expand Up @@ -110,7 +110,7 @@ def test_comments() -> None:
int_opt: 10 ; semicolons matter too
# commented_out: 10
; commented_out: 10
# Comments on new lines should be preserved
; Semicolon comments should also be preserved
[isort] # comments on section headers shouldn't matter because we don't convert sections
Expand All @@ -125,7 +125,7 @@ def test_comments() -> None:
int_opt: 10 ; semicolons matter too
# commented_out: 10
; commented_out: 10
# Comments on new lines should be preserved
; Semicolon comments should also be preserved
[isort] # comments on section headers shouldn't matter because we don't convert sections
Expand Down Expand Up @@ -215,14 +215,14 @@ def test_multiline_options_ignored() -> None:
[GLOBAL]
multiline_string: in a galaxy far,
far, away...
l1: [
'foo',
]
l2: ['foo',
'bar']
d: {
'a': 0,
}
Expand Down
5 changes: 3 additions & 2 deletions build-support/mypy/mypy.ini
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
[mypy]
# TODO(#10131): Re-enable this plugin once we add support to v2 MyPy.
# See: https://mypy.readthedocs.io/en/latest/extending_mypy.html#configuring-mypy-to-use-plugins
plugins =
pants.mypy.plugins.total_ordering
# plugins =
# pants.mypy.plugins.total_ordering

# Refer to https://mypy.readthedocs.io/en/latest/command_line.html for the definition of each
# of these options. MyPy is frequently updated, so this file should be periodically reviewed
Expand Down
1 change: 0 additions & 1 deletion contrib/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
target(
name = 'plugins',
dependencies = [
'contrib/mypy/src/python/pants/contrib/mypy:plugin',
]
)

Expand Down
7 changes: 0 additions & 7 deletions contrib/mypy/BUILD

This file was deleted.

44 changes: 0 additions & 44 deletions contrib/mypy/README.md

This file was deleted.

42 changes: 0 additions & 42 deletions contrib/mypy/examples/src/python/mypy_plugin/BUILD

This file was deleted.

Empty file.
6 changes: 0 additions & 6 deletions contrib/mypy/examples/src/python/mypy_plugin/invalid.py

This file was deleted.

6 changes: 0 additions & 6 deletions contrib/mypy/examples/src/python/mypy_plugin/mypy.ini

This file was deleted.

Loading

0 comments on commit c483495

Please sign in to comment.