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

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

Merged
merged 4 commits into from May 24, 2018

Conversation

@cclauss
Copy link
Contributor

@cclauss 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
Copy link
Contributor

@agjohnson agjohnson left a comment

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
Copy link
Contributor

@agjohnson agjohnson Jan 15, 2018

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

from builtins import str

Correct?

@stsewd stsewd mentioned this pull request Jan 16, 2018
@@ -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)))
Copy link
Contributor

@agjohnson agjohnson Jan 16, 2018

@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?

Copy link
Contributor

@xrmx xrmx Jan 17, 2018

What about six.text_type ?

@cclauss
Copy link
Contributor Author

@cclauss 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
Copy link
Member

@Blendify 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
Copy link
Member

@humitos 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
Copy link
Member

@stsewd stsewd commented May 1, 2018

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

@ericholscher ericholscher merged commit 24a9a0c into readthedocs:master May 24, 2018
1 check passed
@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 24, 2018

Thanks!

@cclauss cclauss deleted the 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants