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

Switch over to using six.PY{2,3} when possible #1416

Merged
merged 2 commits into from Jul 20, 2018

Conversation

Projects
None yet
2 participants
@pganssle
Member

pganssle commented Jul 10, 2018

Summary of changes

Per the reasoning in #1415, I think that we should use six.PY{2,3} whenever possible and outsource how to determine the major version to them.

The one concession I made to #1413 is that I changed one sys.version_info.major to sys.version_info[0] in the version logic itself, because that is more consistent with how six does it. I'm not saying that we guaranteed that we'll never use sys.version_info.major in the future, just that this is another equally good way to check the value that doesn't trigger the Jython bug.

Closes #1415

Pull Request Checklist

  • Changes have tests N/A
  • News fragment added in changelog.d. See documentation for details
@@ -1,6 +1,5 @@
import sys

This comment has been minimized.

@pganssle

pganssle Jul 10, 2018

Member

Oops, I think this was a mistake. I think this is a PEP 8 violation. I'll clean it up.

pganssle added some commits Jul 10, 2018

@@ -48,7 +48,7 @@ def load_module(self, fullname):
# on later Python versions to cause relative imports
# in the vendor package to resolve the same modules
# as those going through this importer.
if sys.version_info.major >= 3:
if sys.version_info >= (3, ):

This comment has been minimized.

@thisch

thisch Jul 10, 2018

Can be changed to if not six.PY2

This comment has been minimized.

@pganssle

pganssle Jul 10, 2018

Member

I think not, because this code is for loading the vendored packages and six is one of the vendored packages it's loading. I could be wrong, but I think it's not possible to use the vendored packages in extern/__init__.py

This comment has been minimized.

@thisch

thisch Jul 10, 2018

Sry, I didn't know that six can't be used in this module.

@pganssle

This comment has been minimized.

Member

pganssle commented Jul 12, 2018

@jaraco Any objections? I know you didn't want to explicitly handle the Jython case.

@pganssle

This comment has been minimized.

Member

pganssle commented Jul 20, 2018

I'm going to merge this.

@pganssle pganssle merged commit 5f51091 into pypa:master Jul 20, 2018

5 checks passed

codecov/patch 100% of diff hit (target 81.1%)
Details
codecov/project 81.1% (+<.01%) compared to 54ce659
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment