Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup PexInfo and PythonInterpreter. #581

Merged
merged 1 commit into from
Oct 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions pex/interpreter.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,14 +293,6 @@ class PythonInterpreter(object):

CACHE = {} # memoize executable => PythonInterpreter

try:
# Versions of distribute prior to the setuptools merge would automatically replace
# 'setuptools' requirements with 'distribute'. It provided the 'replacement' kwarg
# to toggle this, but it was removed post-merge.
COMPATIBLE_SETUPTOOLS = Requirement.parse('setuptools>=1.0', replacement=False)
except TypeError:
COMPATIBLE_SETUPTOOLS = Requirement.parse('setuptools>=1.0')

class Error(Exception): pass
class IdentificationError(Error): pass
class InterpreterNotFound(Error): pass
Expand Down
9 changes: 4 additions & 5 deletions pex/pex_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import json
import os
import warnings
from collections import namedtuple

from .common import open_zip
from .compatibility import PY2
Expand All @@ -15,8 +14,6 @@
from .util import merge_split
from .variables import ENV

PexPlatform = namedtuple('PexPlatform', 'interpreter version strict')


# TODO(wickman) Split this into a PexInfoBuilder/PexInfo to ensure immutability.
# Issue #92.
Expand Down Expand Up @@ -53,13 +50,15 @@ class PexInfo(object):
@classmethod
def make_build_properties(cls, interpreter=None):
from .interpreter import PythonInterpreter
from pkg_resources import get_platform
from .platforms import Platform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relative import initially made me uncomfortable because I know they can be problematic for things like refactoring, and the Py2 syntax breaks on Py3. But I reviewed the book Python Cookbook and they say it's totally valid + you're copying the precedent, so I think it's fine.

You're also using the Py3 syntax, which resolves the biggest concern I would have.


pi = interpreter or PythonInterpreter.get()
plat = Platform.current()
platform_name = plat.platform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this variable name. Definitely makes things more clear than platform: plat.platform

return {
'class': pi.identity.interpreter,
'version': pi.identity.version,
'platform': get_platform(),
'platform': platform_name,
}

@classmethod
Expand Down