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

Travis CI: Fix the remaining Python 3 issues #1793

Closed
wants to merge 2 commits into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jun 21, 2019

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

Resolve issues related to the following items being removed from Python 3:

  • basestring
  • cmp()
  • long
  • reload() (moved, not removed)
  • sys.setdefaultencoding()
  • unicode

@rvagg
Copy link
Member

rvagg commented Jun 22, 2019

sweet, @nodejs/python can we get at least one other python person to +1 this please? then we can get it shipped.

@cclauss cclauss requested a review from bnoordhuis June 24, 2019 10:28
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

rvagg pushed a commit that referenced this pull request Jun 26, 2019
PR-URL: #1793
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Jun 26, 2019
PR-URL: #1793
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg
Copy link
Member

rvagg commented Jun 26, 2019

landed in master

@cclauss just for educational purposes so you can start landing these yourself: I've rewritten both commits to prepend a "subsystem" like we do in nodejs/node. The first commit is "gyp: ..." and the second is "test: ...". These prefixes make the changelog a little more digestable and our changelog-maker tool can group them. I've also added the following metadata to the commits, again for our tools (changelog-maker and branch-diff):

PR-URL: https://github.com/nodejs/node-gyp/pull/1793
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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