Skip to content

Commit

Permalink
Review feedback:
Browse files Browse the repository at this point in the history
* magic numbers
* talk about MYPYPATH
* try to make warning more actionable
* typo
* ensure we only log one time

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano committed Sep 25, 2020
1 parent 9cebfa1 commit 9dc0981
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 21 deletions.
39 changes: 21 additions & 18 deletions src/python/pants/backend/python/typecheck/mypy/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,27 +88,27 @@ def generate_argv(
def check_and_warn_if_python_version_configured(
*, config: Optional[FileContent], args: Tuple[str, ...]
) -> bool:
ending = (
"Normally, Pants would automatically set this for you based on your code's interpreter "
"constraints (https://www.pantsbuild.org/docs/python-interpreter-compatibility).\n\n(This "
"allows Pants to partition your targets by their constraints, so that, for example, "
"you can run MyPy on Python 2-only code and Python 3-only code at the same time. This "
"feature may no longer work.)"
)
configured = False
configured = []
if config and b"python_version" in config.content:
logger.warning(
f"You set `python_version` in {config.path}, which is used because of the "
f"`[mypy].config` option. {ending}"
configured.append(
f"`python_version` in {config.path} (which is used because of the "
"`[mypy].config` option)"
)
configured = True
if "--py2" in args:
logger.warning(f"You set `--py2` in the `--mypy-args` option. {ending}")
configured = True
configured.append("`--py2` in the `--mypy-args` option")
if any(arg.startswith("--python-version") for arg in args):
logger.warning(f"You set `--python-version` in the `--mypy-args` option. {ending}")
configured = True
return configured
configured.append("`--python-version` in the `--mypy-args` option")
if configured:
formatted_configured = " and you set ".join(configured)
logger.warning(
f"You set {formatted_configured}. Normally, Pants would automatically set this for you "
"based on your code's interpreter constraints "
"(https://www.pantsbuild.org/docs/python-interpreter-compatibility). Instead, it will "
"use what you set.\n\n(Automatically setting the option allows Pants to partition your "
"targets by their constraints, so that, for example, you can run MyPy on Python 2-only "
"code and Python 3-only code at the same time. This feature may no longer work.)"
)
return bool(configured)


def config_path_globs(mypy: MyPy) -> PathGlobs:
Expand Down Expand Up @@ -192,7 +192,7 @@ async def mypy_typecheck_partition(partition: MyPyPartition, mypy: MyPy) -> Type

# MyPy requires 3.5+ to run, but uses the typed-ast library to work with 2.7, 3.4, 3.5, 3.6,
# and 3.7. However, typed-ast does not understand 3.8, so instead we must run MyPy with
# Python 3.8 when relevant. We only do this if if <3.8 can't be used, as we don't want a
# Python 3.8 when relevant. We only do this if <3.8 can't be used, as we don't want a
# loose requirement like `>=3.6` to result in requiring Python 3.8, which would error if
# 3.8 is not installed on the machine.
tool_interpreter_constraints = PexInterpreterConstraints(
Expand All @@ -218,6 +218,9 @@ async def mypy_typecheck_partition(partition: MyPyPartition, mypy: MyPy) -> Type
# Instead, we teach MyPy about the requirements by extracting the distributions from
# requirements.pex and setting EXTRACTED_WHEELS, which our custom launcher script then
# looks for.
#
# Conventionally, MyPy users might instead set `MYPYPATH` for this. However, doing this
# results in type checking the requirements themselves.
requirements_pex_request = Get(
Pex,
PexFromTargetsRequest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -680,3 +680,12 @@ def assert_is_configured(*, has_config: bool, args: List[str], warning: str) ->
args=["--python-version=3.6"],
warning="You set `--python-version` in the `--mypy-args` option",
)
assert_is_configured(
has_config=True,
args=["--py2", "--python-version=3.6"],
warning=(
"You set `python_version` in mypy.ini (which is used because of the `[mypy].config` "
"option) and you set `--py2` in the `--mypy-args` option and you set "
"`--python-version` in the `--mypy-args` option."
),
)
7 changes: 4 additions & 3 deletions src/python/pants/backend/python/util_rules/pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ def includes_python2(self) -> bool:
This will return True even if the code works with Python 3 too, so long as at least one of
the constraints works with Python 2.
"""
return self._includes_version("2.7", last_patch=18)
last_py27_patch_version = 18
return self._includes_version("2.7", last_patch=last_py27_patch_version)

def minimum_python_version(self) -> Optional[str]:
"""Find the lowest major.minor Python version that will work with these constraints.
Expand All @@ -226,9 +227,9 @@ def minimum_python_version(self) -> Optional[str]:
"""
if self.includes_python2():
return "2.7"
max_expected_py3_patch_version = 12 # The current max is 9.
for major_minor in ("3.5", "3.6", "3.7", "3.8", "3.9", "3.10"):
# Assume any 3.x release has no patch after 12. The max is currently 9.
if self._includes_version(major_minor, last_patch=12):
if self._includes_version(major_minor, last_patch=max_expected_py3_patch_version):
return major_minor
return None

Expand Down

0 comments on commit 9dc0981

Please sign in to comment.