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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

eirnym
Copy link

@eirnym eirnym commented Oct 14, 2018

C

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Continuation of #1335 and #1150. Please, review, comment, add more changes and fixes :) All changes are splitted, as some of them are massive.

UPDATE 1
Now I'm fighting with test/test-find-python.js file to allow it even to start testing in Python3-only environment.

UPDATE 2
Tests are passing, but it's possible that I'd miss something

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Great stuff... Just two minor change requests. Thanks.

for x in (type(None), int, long, float,
bool, str, unicode, type):
import sys
if sys.version_info < (3, 0):
Copy link
Contributor

Choose a reason for hiding this comment

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

On this one, can we please follow Python porting best practice use feature detection instead of version detection?

Copy link
Author

Choose a reason for hiding this comment

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

extracted checks to compat module

@@ -155,6 +155,9 @@
_new_sha1 = sha.new


if sys.version_info >= (3, 0): # just easier
unicode = str
Copy link
Contributor

Choose a reason for hiding this comment

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

On this one, can we please follow Python porting best practice use feature detection instead of version detection?

Copy link
Author

Choose a reason for hiding this comment

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

There's a reason for it in this case: exception -> substitution will run much slower than knowlege of version where this name doesn't exist. This is also used in six and works well.

Copy link
Author

@eirnym eirnym Oct 14, 2018

Choose a reason for hiding this comment

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

so I'd prefer to leave this kind of check for types and for a dictionary. But I can change if you'd insist on it.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

I'm going to land @rodrigc's #1150, then I'll rebase this to see what's left.

@refack refack self-assigned this Oct 14, 2018
@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

@refack it won't work as I found a lot more issues in code to be ported

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

This doesn't mean, his work is useless.

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

@refack @cclauss Could you have a chance to know what these variables should be? I haven't found any traces of them in functions they are and how to fix these lines.

./gyp/pylib/gyp/xcode_emulation.py:846:9: F821 undefined name 'cflags'
        cflags.append('-F' + platform_root + '/Developer/Library/Frameworks/')
        ^
./gyp/pylib/gyp/generator/make.py:1645:50: F821 undefined name 'target'
      print("WARNING: no output for", self.type, target)
                                                 ^
2     F821 undefined name 'target'
2

@cclauss
Copy link
Contributor

cclauss commented Oct 14, 2018

See #1333 and #1334 My sense on cflags is that it should be ldflags in this context but not 100% certain.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

it won't work as I found a lot more issues in code to be ported

As long as it passes CI, I take it as a sign it doesn't regress. I assumed that work was not done yet to get actual py3 compat.
Let's see is we can get everything done in this PR.

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

My PR also passes CI, but not finished :D There's many things to fix

Before you try to merge it, I'd like to finish some other checks. I'll let you know

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

Merged and I've done for today.

@cclauss #1333 is included

Unresolved questions for the next run are:

  • sorted() and cmp=
  • input.py: compile.ast -> ast. It's about to rewrite main part of the module.
  • optparse may be dropped in a foreseeable future
  • mac_tool uses veeery old version of plistlib and implies that pyobjc is installed (which may be not if we install python from MacPorts/Homebrew)
  • I crashed pylint

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

@refack I finished, including merging

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

@cclauss for this run I used pylint --disable=E1125,E0702,E0702,E1701,E1124,C,W0311 pylib after flake8

@refack
Copy link
Contributor

refack commented Oct 14, 2018

@eirnym I've rebased you branch. Before you continue, take a tag, and pull with --rebase, so you can compare what changed.

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

you lost at least a half of my changes during rebasing. Merge would be always easier.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

you lost at least a half of my changes during rebasing. Merge would be always easier.

We keep a linier git history, so we rebase. Some changes were incompatible with #1572 so I did not base them.

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

If you'd tell me before, It would be just easier to rebase them manually. Now I repeat my work.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

you lost at least a half of my changes during rebasing. Merge would be always easier.

Sorry. I'm juggling several branches, and reviewing a ton of code...

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

I understand, but tell about this to a developer to save their time. It's already half past midnight and I'm restoring changes for an hour by now instead going to sleep.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

I understand, but tell about this to a developer to save their time. It's already half past midnight and I'm restoring changes for an hour by now instead going to sleep.

Sorry again.
There is no rush... this could wait a few more days.

Thank you for the dedication.

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

Next possible day will be in a few days and I don't want to forget and start all things over again.

@eirnym
Copy link
Author

eirnym commented Oct 14, 2018

I restored changes. Commit is a bit bulky, but it does it's job right.

@refack
Copy link
Contributor

refack commented Oct 14, 2018

Thank you very much 🥇

@eirnym
Copy link
Author

eirnym commented Oct 18, 2018

So what the current state of the PR?

@eirnym
Copy link
Author

eirnym commented Oct 20, 2018

@refack @cclauss

@cclauss
Copy link
Contributor

cclauss commented Oct 21, 2018

Conflicts.

I have approved but I am not a maintainer. Overall, I would feel more comfortable if we used the six module (or futurize) instead of inventing our own from scratch. The linter directives in gyp/pylib/gyp/compat.py are just as a result of a refusal to follow Python porting best practice use feature detection instead of version detection. The map()s and filter()s would be faster and more readable as list comprehensions.

@eirnym
Copy link
Author

eirnym commented Oct 21, 2018

@cclauss I feel the same about six, but I'm not a maintainer to make a decision about additional dependency

Checking Python version is significantly faster and stable enough and it's used even in pip and six.

about map and filter. I changed as little as it was possible. "this is not making code clean and fast, but only a compatibility in mind". if you start make this optimisations, you'll end up with rewriting the whole code from scratch leaving only required features. This is a huge work and I'm not a person to take it in 100%.

@eirnym
Copy link
Author

eirnym commented Oct 21, 2018

And please, remember that we have no tests. npm run test is not sufficient here.

@eirnym
Copy link
Author

eirnym commented Oct 21, 2018

One more thing about compatibility and Python version checking: this is only about syntax and builtins checking. For Python2 you can't use async/await as well as for Python 3 you can't use print statement and very old variant of raising. With imports it's try-except impoting which is basically feautre-checking and never monkey-patching.

@eirnym
Copy link
Author

eirnym commented Oct 25, 2018

@refack almost 2 weeks passed since I done this work. Please, tell me what else needs to be done here?

@cclauss
Copy link
Contributor

cclauss commented Nov 15, 2018

Conflicts :-(

@eirnym
Copy link
Author

eirnym commented Nov 18, 2018

@cclauss it's easy to solve code conflicts, but I don't see any intention from maintainers to merge these changes anyway.

@eirnym eirnym closed this Nov 18, 2018
@eirnym
Copy link
Author

eirnym commented Nov 18, 2018

I gave up on this branch. I have a copy if anybody would be intereseted in merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants