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

Modernize Python 2 code to get ready for Python 3 #3514

Merged
merged 4 commits into from May 24, 2018

Conversation

Projects
None yet
8 participants
@cclauss
Contributor

cclauss commented Jan 13, 2018

Make the minimal, safe changes required to convert the repo's code to be syntax compatible with both Python 2 and Python 3. There may be other changes required to complete a port to Python 3 but this PR is a minimal, safe first step.

Run: futurize --stage1 -w **/*.py

See Stage 1: "safe" fixes http://python-future.org/automatic_conversion.html#stage-1-safe-fixes
@agjohnson

Thanks! Changes to files in deploy/ look good, but we should probably just remove the files there that we don't use anymore. We can address that in a separate PR though.

try:
unicode # Python 2
except NameError:
unicode = str # Python 3

This comment has been minimized.

@agjohnson

agjohnson Jan 15, 2018

Contributor

We already have a pattern for this using future. I believe this should just be:

from builtins import str

Correct?

@stsewd stsewd referenced this pull request Jan 16, 2018

Closed

Py3 incompatibility #3516

@@ -168,7 +163,7 @@ def save_environment_json(self):
with open(self.environment_json_path(), 'w') as fpath:
# Compatibility for Py2 and Py3. ``io.TextIOWrapper`` expects
# unicode but ``json.dumps`` returns str in Py2.
fpath.write(unicode(json.dumps(data)))
fpath.write(str(json.dumps(data)))

This comment has been minimized.

@agjohnson

agjohnson Jan 16, 2018

Contributor

@humitos explains that this might not be the fix we want:

#3516 (comment)

Do we need to write via a handle instantiated through codecs perhaps?

This comment has been minimized.

@xrmx

xrmx Jan 17, 2018

Contributor

What about six.text_type ?

@cclauss

This comment has been minimized.

Contributor

cclauss commented Feb 1, 2018

Given these comments #3514 (review) should we just close this PR? Is there a PR to remove the /deploy files?

@Blendify

This comment has been minimized.

Contributor

Blendify commented Feb 5, 2018

I think it would be fine to merge this and remove the files we don't use in a separate PR still. (No this PR does not exist yet).

@humitos

This comment has been minimized.

Member

humitos commented Mar 23, 2018

Given these comments #3514 (review) should we just close this PR? Is there a PR to remove the /deploy files?

I think you can just remove this files in this PR and we can merge.

@stsewd

This comment has been minimized.

Member

stsewd commented May 1, 2018

Hi @cclauss, are you still interested in finishing this PR?

@ericholscher ericholscher merged commit 24a9a0c into rtfd:master May 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

Thanks!

@cclauss cclauss deleted the cclauss:modernize-python2-code branch May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment