Add type hints to most tests #1036
Conversation
| @@ -29,8 +29,6 @@ | |||
| PY3 = sys_version_info[0] == 3 | |||
|
|
|||
| string = (str,) if PY3 else (str, unicode) # type: ignore[name-defined] | |||
| unicode_string = (str,) if PY3 else (unicode,) # type: ignore[name-defined] | |||
| bytes = (bytes,) # type: ignore[has-type] | |||
There was a problem hiding this comment.
This shadowed the builtin bytes, which messed up below return types. It was also unused.
| @@ -29,8 +29,6 @@ | |||
| PY3 = sys_version_info[0] == 3 | |||
|
|
|||
| string = (str,) if PY3 else (str, unicode) # type: ignore[name-defined] | |||
| unicode_string = (str,) if PY3 else (unicode,) # type: ignore[name-defined] | |||
There was a problem hiding this comment.
Only used in tests for this file. Now inlined.
| yield run_simple_pex(pex_path, env={"PEX_VERBOSE": "1"})[0] | ||
| stdout, _ = run_simple_pex(pex_path, env={"PEX_VERBOSE": "1"}) | ||
| yield stdout.decode("utf-8") |
There was a problem hiding this comment.
We didn't call .decode() before. Instead, we used str() on the object at each callsite. This is more explicit
|
|
||
| def create_sdist(*args, **kwargs): |
There was a problem hiding this comment.
Only kwargs were being used.
| return Requirement.parse(str(req)) | ||
| # type: (Union[str, Requirement]) -> Requirement | ||
| if isinstance(req, Requirement): | ||
| req = str(req) | ||
| return Requirement.parse(req) |
There was a problem hiding this comment.
This was changed to be more explicit.
| @@ -200,7 +203,7 @@ def run(): | |||
| "--always-write-cache", | |||
| "--pex-root", | |||
| ptex_cache, | |||
| pex_project_dir, | |||
| pex_project_dir, # type: ignore[list-item] # This is unicode in Py2, whereas everthing else is bytes. That's fine. | |||
| empty_hash = sha1().hexdigest() | ||
| # type: () -> None | ||
| empty_hash_digest = sha1().hexdigest() |
There was a problem hiding this comment.
MyPy didn't like that the variable was being reassigned later with a different type.
We especially don't want to start changing production code like this.
| @@ -51,8 +56,6 @@ class PexInfo(object): | |||
|
|
|||
| @classmethod | |||
| def make_build_properties(cls, interpreter=None): | |||
| from .interpreter import PythonInterpreter | |||
There was a problem hiding this comment.
This import may have been intentionally lazy (although it's not written down). I might recommend keeping it in this block, although making it absolute instead of relative.
There was a problem hiding this comment.
I was looking if the lazy import would actually end up mattering in production. It looks like no. It doesn't impact the imports of this file; if you only ever import pex_info.py, then you end up transitively importing way more than before.
But the only two places we import this file in production already import a ton of other things, resolver.py and pex/pex.py. So this shouldn't matter.
There was a problem hiding this comment.
In general, I would normally say if there's no comment, feel free to kill it, but I'm concerned about performance regressions since @jsirois isn't around. If he could make sure of that when he gets back that would be great, but otherwise we can 🚢 for now!
Co-authored-by: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com>
Co-authored-by: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com>
| @@ -51,8 +56,6 @@ class PexInfo(object): | |||
|
|
|||
| @classmethod | |||
| def make_build_properties(cls, interpreter=None): | |||
| from .interpreter import PythonInterpreter | |||
There was a problem hiding this comment.
In general, I would normally say if there's no comment, feel free to kill it, but I'm concerned about performance regressions since @jsirois isn't around. If he could make sure of that when he gets back that would be great, but otherwise we can 🚢 for now!
Now all tests have full coverage except for
test_environment.py,test_pex.py, andtest_integration.py, which are saved for a dedicated followup.Why start with tests? These are the call-sites of much of our production code, and this will help to ensure that when we add types to our production code, the hints reflect reality. The tests also are "roots", which makes it easier to add hints to than adding hints to code with lots of dependees.
strvs.bytesvs.Textvs.AnyStrPython 2 lets you freely mix unicode and bytes :/ Pex uses this abundantly, in contrast to Pants's (awkward) use of
futureto ensure that Py2 always used the exact same type as Py3.Originally, this PR added
from __future__ import unicode_literals, but that was reverted to instead optimize for fewer code changes. PEX's Py2 support works well as is, and it's well tested. It's not worth the energy to do a Pants-style migration that ensures Py2 behaves like Py3.Instead, these hints try to capture the truth, as follows:
bytes-> when both Py2 and Py3 usebytesstr-> when Py2 usesbytes, but Py3 uses unicodeText-> when both use unicode. This only really happens when we explicitly call.decode('utf-8')AnyStr-> when it can be either unicode or bytes; shorthand forUnionOnce we require Py3 to run,
TextandAnyStrwill both be converted into simplystr(i.e. unicode).