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 #1335

Merged
merged 1 commit into from
Oct 14, 2018
Merged

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

merged 1 commit into from
Oct 14, 2018

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 14, 2017

Similar to #1150

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

Checklist
Description of change

@cclauss cclauss mentioned this pull request Nov 14, 2017
@bertonha
Copy link

bertonha commented Jan 9, 2018

Any news on this PR?

@Stanzilla
Copy link

any interest by the maintainers to get this done?

@cclauss
Copy link
Contributor Author

cclauss commented May 13, 2018

@refack Your review please.

Copy link

@DatNoHand DatNoHand left a comment

Choose a reason for hiding this comment

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

nicely done, they should merge this

@cclauss
Copy link
Contributor Author

cclauss commented Aug 24, 2018

495 days until Python 2 end of life. https://bugs.chromium.org/p/gyp/issues/detail?id=36#c48

@cclauss
Copy link
Contributor Author

cclauss commented Aug 24, 2018

If anyone is interested in this PR then please submit a review:

  1. Click on Files changed near the top of this page
  2. Click on Review changes
  3. Select Approve or Request changes
  4. Click Submit review

The more approvals there are, the more confidence that Maintainers will have that these changes are worth making.

@refack
Copy link
Contributor

refack commented Aug 24, 2018

I was waiting for the maintainer of upstream GYP to do something similar, but that probably won't happen.
@cclauss I'll be happy to review your work.

P.S. I have been working on reviving the GYP test suite from upstream. I feel it's important to have some regression testing so we'll be more confident this change is safe.

@refack refack self-assigned this Aug 24, 2018
@cclauss
Copy link
Contributor Author

cclauss commented Aug 24, 2018

What is the URL to the repo for "upstream GYP"? https://gyp.gsrc.io/ or other?

@refack
Copy link
Contributor

refack commented Aug 24, 2018

@cclauss
Copy link
Contributor Author

cclauss commented Oct 8, 2018

@refack Can we please proceed with the review of this PR? Having this repo ready for the shift from legacy Python to Python will put more pressure on the maintainers of "upstream GYP" to get with the times. 450 days until Python 2 EOL.

@Croydon
Copy link

Croydon commented Oct 8, 2018

I guess the idea is that GN replaces GYP so we will never see major upstream GYP changes. I assume that Node needs to switch to something else entirely in the long run, but there are enough open issues for that, so we shouldn't get into this here...

@cclauss
Copy link
Contributor Author

cclauss commented Oct 8, 2018

Is o-lim/generate-ninja#1 the canonical repo for GN?

@Croydon
Copy link

Croydon commented Oct 9, 2018

GN is, originally, part of the Chromium source tree. However, this is a fork of the original GN, which has been extracted from the Chromium tree so that it may be built standalone.

It is a fork.

@cclauss
Copy link
Contributor Author

cclauss commented Oct 9, 2018

Does upstream score better on those tests than the fork does?

@eirnym
Copy link

eirnym commented Oct 13, 2018

Please, update README as well if it works with Python 3

@cclauss
Copy link
Contributor Author

cclauss commented Oct 14, 2018

Let’s not change README until we get more feedback from users. No syntax errors != fully ported.

@eirnym
Copy link

eirnym commented Oct 14, 2018

I had no errors for a while by now… with python2

@eirnym
Copy link

eirnym commented Oct 14, 2018

please, run 2to3 again

@cclauss
Copy link
Contributor Author

cclauss commented Oct 14, 2018

2to3 is a bad idea because it often breaks Python 2 compatibility to achieve Python 3 compatibility. This PR was created with futurize, not 2to3.

@eirnym
Copy link

eirnym commented Oct 14, 2018

I didn't said you need to apply it blindliy, but it's a good idea to highlight things.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

I'll review today.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

P.S. what I have been trying to do was revive the original GYP test suite as to have some gauge of the compatibility of this change.
If anyone can help with adding some .gyp based test cases, or suggesting existing addons that might be interesting to check against, that would be very helpful.

@eirnym
Copy link

eirnym commented Oct 14, 2018

a few notes on 2to3:

  • range is usually fine even on Python2, on Python 3 it's better than xrange was.
  • for each broken import or relocated module, example code below is usually used
  • prefer import python3 modules first as most systems currently install python3 as default. The only difference is macOS, but most developers will install nodejs from macports or homebrew.
  • if you're really struggling with syntax or importing stuff, could read source of six module.
try:
    import urllib.parse as parse
except ImportError:
    import urlparse as parse       # name have to be the same (in most cases)
try:
    from io import StringIO
except ImportError:
    from StringIO import StringIO

@cclauss
Copy link
Contributor Author

cclauss commented Oct 14, 2018

Does gyp pass these two tests?

  • python2 -m flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics
  • python3 -m flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics

@eirnym
Copy link

eirnym commented Oct 14, 2018

it's not recommended if your app is long living (not node-gyp) and lists contain millions of records.

For compatible code for this particular project it's more convinient to use one function everywhere.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/90/
https://ci.nodejs.org/job/nodegyp-test-pull-request/91/ ✔️

#1335
Reviewed-By: Refael Ackermann <[email protected]>
@refack refack merged commit c9276f3 into nodejs:master Oct 14, 2018
@refack
Copy link
Contributor

refack commented Oct 14, 2018

Happy futuring y'all

@cclauss cclauss deleted the modernize-python2-code branch October 14, 2018 16:10
@eirnym
Copy link

eirnym commented Oct 14, 2018

Added another PR with more fixes. You're welcome to review, comment and add more changes there.

@refack refack mentioned this pull request Nov 19, 2018
refack pushed a commit to refack/GYP3 that referenced this pull request Dec 2, 2018
refack pushed a commit to refack/GYP3 that referenced this pull request Dec 2, 2018
refack pushed a commit to refack/GYP3 that referenced this pull request Jan 29, 2019
refack pushed a commit to refack/GYP3 that referenced this pull request Jan 29, 2019
refack pushed a commit to refack/GYP3 that referenced this pull request Feb 7, 2019
rvagg pushed a commit that referenced this pull request Apr 24, 2019
#1335
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants