Add type hints to variables.py and platforms.py #1042

Eric-Arellano · 2020-09-25T17:03:19Z

Unfortunately, so long as we require Python 2, we cannot use namedtuple robustly with MyPy because it does not support annotating the fields of a named tuple. So, we recreate namedtuple through our own conventional class. This can be fixed once we require Python 3 to run. See #1041.

Eric-Arellano · 2020-09-25T17:04:22Z

pex/dist_metadata.py



 def requires_python(dist):
+    # type: (Distribution) -> Optional[SpecifierSet]


Note that MyPy doesn't actually know what these are due to the vendoring. But I believe that it will at least check we are using type A, rather than type Z; it just doesn't understand the methods/fields on the type.

And this helpful for the reader.

We can possibly create stubs for these if needed?

Oh, that'd be interesting as a followup once we have higher coverage. pkg_resources is pretty key to Pex. Having type hints working with Pex would be great.

Eric-Arellano · 2020-09-25T17:05:31Z

pex/pex_warnings.py


 class PEXWarning(Warning):
    """Indicates a warning from PEX about suspect buildtime or runtime configuration."""


 def configure_warnings(pex_info, env):
-    if env.PEX_VERBOSE > 0:
+    # type: (PexInfo, Variables) -> None
+    if env.PEX_VERBOSE is not None and env.PEX_VERBOSE > 0:


This was a bug, although I don't know if we ever hit it. It's possible for PEX_VERBOSE to be None, such as if you call ENV.strip() and the user did not define the value.

There is a bug lurking but its unhittable:
https://github.com/pantsbuild/pex/blob/aae2a6ad55e148ada43f36c1d486af9b2312af50/pex/variables.py#L364-L372
https://github.com/pantsbuild/pex/blob/aae2a6ad55e148ada43f36c1d486af9b2312af50/pex/variables.py#L104-L113

... combined with use_defaults is True for the global ENV which is what the only use of configure_warnings in production passes (all of production only uses the global ENV).

Eric-Arellano · 2020-09-25T17:06:10Z

pex/platforms.py

+
+    def __init__(self, platform, impl, version, abi):
+        super(Platform, self).__init__()
+        if not all((platform, impl, version, abi)):


It'd be better to check if these are str, rather than simply being truthy. But I tried to minimize my changes. Lmk if I should add it.

I think we might want to make abi at least into an enum when we can drop py2, although I'm not sure checking for str now makes it harder to do later. I think leaving it as you've got it here sounds good.

pex/platforms.py

Eric-Arellano · 2020-09-25T17:07:20Z

pex/variables.py

        self._use_defaults = use_defaults
-        self._environ = (environ if environ is not None else os.environ).copy()
+        self._environ = environ.copy() if environ is not None else os.environ.copy()


MyPy was getting confused trying to find the common denominator between our Dict vs. the _OSEnviron type, then complaining that .copy() wasn't define on that common interface of MutableMapping. This fixes it.

Eric-Arellano · 2020-09-25T17:07:42Z

pex/variables.py

-        else:
-            return self._defaulted(default)
+        if value is None:
+            return self._defaulted(default)  # type: ignore[return-value]


We have to ignore due to the imprecise ._defaulted method.

Might this be the place for a cast()?

We can't because it returns Optional[bool] :/

That's quite annoying.

Noted in #1042, but you can use cast, just pass the types as strings when those types (or aliases) are only available at TYPE_CHECKING time.

Eric-Arellano · 2020-09-25T17:08:07Z

pex/variables.py

        value = self._environ.get(variable)
-        if value is not None:


This method is the same code as before, only refactored by inverting the conditional and early returns.

Eric-Arellano · 2020-09-25T17:08:41Z

pex/variables.py

+        # type: () -> Optional[bool]
        """Boolean.

        Enable application profiling.  If specified and PEX_PROFILE_FILENAME is not specified, PEX
        will print profiling information to stdout.
        """
-        return self._get_path("PEX_PROFILE", default=None)
+        return self._get_bool("PEX_PROFILE", default=False)


This was confusing to me. I think a bug. We document it as a bool, but treat it as a path. @jsirois lmk if you know what we intended.

LGTM.

I don't know but git does. See 0fd58f3 via git log -S PEX_PROFILE_FILENAME.

It looks like an oversight in that change to switch from path to bool when PEX_PROFILE_FILENAME was introduced / split out to parallel PEX_COVERAGE*.

pex/platforms.py

cosmicexplorer · 2020-09-25T17:10:25Z

pex/dist_metadata.py



 def requires_python(dist):
+    # type: (Distribution) -> Optional[SpecifierSet]


We can possibly create stubs for these if needed?

cosmicexplorer · 2020-09-25T17:12:47Z

pex/variables.py

@@ -21,15 +25,17 @@ class Variables(object):

    @classmethod
    def process_pydoc(cls, pydoc):
+        # type: (Optional[str]) -> Tuple[str, str]
        if pydoc is None:


Possibly consider extending this to extract type hints in the future.

Once we require Python 3, that would be super easy to do with typing.get_type_hints(), which is how the Pants rule signature parsing code works.

jsirois · 2020-09-25T18:50:35Z

pex/pex_warnings.py


 class PEXWarning(Warning):
    """Indicates a warning from PEX about suspect buildtime or runtime configuration."""


 def configure_warnings(pex_info, env):
-    if env.PEX_VERBOSE > 0:
+    # type: (PexInfo, Variables) -> None
+    if env.PEX_VERBOSE is not None and env.PEX_VERBOSE > 0:


There is a bug lurking but its unhittable:
https://github.com/pantsbuild/pex/blob/aae2a6ad55e148ada43f36c1d486af9b2312af50/pex/variables.py#L364-L372
https://github.com/pantsbuild/pex/blob/aae2a6ad55e148ada43f36c1d486af9b2312af50/pex/variables.py#L104-L113

... combined with use_defaults is True for the global ENV which is what the only use of configure_warnings in production passes (all of production only uses the global ENV).

pex/platforms.py

jsirois · 2020-09-25T18:59:30Z

pex/variables.py

+        # type: () -> Optional[bool]
        """Boolean.

        Enable application profiling.  If specified and PEX_PROFILE_FILENAME is not specified, PEX
        will print profiling information to stdout.
        """
-        return self._get_path("PEX_PROFILE", default=None)
+        return self._get_bool("PEX_PROFILE", default=False)


LGTM.

I don't know but git does. See 0fd58f3 via git log -S PEX_PROFILE_FILENAME.

It looks like an oversight in that change to switch from path to bool when PEX_PROFILE_FILENAME was introduced / split out to parallel PEX_COVERAGE*.

We don't fix IntegResults, though. That's test only, and we don't seem to need any of the generated dunders. It would be lots of boilerplate to go back. In contrast, `Platform` is used in production so worth the boilerplate.

pex/platforms.py

Add type hints to variables.py and platforms.py

7b55c0b

Eric-Arellano requested review from jsirois, tdyas and cosmicexplorer September 25, 2020 17:03

Eric-Arellano commented Sep 25, 2020

View reviewed changes

cosmicexplorer approved these changes Sep 25, 2020

View reviewed changes

Use cast()

bea891e

jsirois approved these changes Sep 25, 2020

View reviewed changes

Go back to using namedtuple for Platform

c603736

We don't fix IntegResults, though. That's test only, and we don't seem to need any of the generated dunders. It would be lots of boilerplate to go back. In contrast, `Platform` is used in production so worth the boilerplate.

jsirois reviewed Sep 25, 2020

View reviewed changes

pex/platforms.py Outdated Show resolved Hide resolved

Use cast

ab7e9b6

Eric-Arellano merged commit b921723 into pex-tool:master Sep 25, 2020

Eric-Arellano deleted the type-hints branch September 25, 2020 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints to variables.py and platforms.py #1042

Add type hints to variables.py and platforms.py #1042

Eric-Arellano commented Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

Eric-Arellano Sep 25, 2020

Eric-Arellano Sep 25, 2020

jsirois Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

Eric-Arellano Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

jsirois Sep 26, 2020

Eric-Arellano Sep 25, 2020

Eric-Arellano Sep 25, 2020

jsirois Sep 25, 2020

cosmicexplorer Sep 25, 2020

cosmicexplorer Sep 25, 2020

Eric-Arellano Sep 25, 2020

jsirois Sep 25, 2020

jsirois Sep 25, 2020



		def requires_python(dist):
		# type: (Distribution) -> Optional[SpecifierSet]

Add type hints to variables.py and platforms.py #1042

Add type hints to variables.py and platforms.py #1042

Conversation

Eric-Arellano commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment