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

build: import StringIO on both Python 2 and Python 3 #1836

Closed
wants to merge 2 commits into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jul 21, 2019

This change supports access to StringIO on both Python 2 and Python 3. Without this change the import fails on Python 3.

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

@cclauss cclauss added the Python label Jul 21, 2019
This change supports access to StringIO on both Python 2 and Python 3
@cclauss
Copy link
Contributor Author

cclauss commented Jul 22, 2019

Do not merge. Similar work to do on other files.

@cclauss cclauss changed the title build: enable ninja.py it import StringIO on Python 3 build: import StringIO on both Python 2 and Python 3 Jul 23, 2019
@@ -8,7 +8,6 @@

import gyp.generator.ninja as ninja
import unittest
import StringIO
Copy link
Member

Choose a reason for hiding this comment

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

Does it not need to be replaced here?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, looks like it isn't actually used here.

Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

Lgtm.

joaocgreis pushed a commit that referenced this pull request Jul 23, 2019
This change supports access to StringIO on both Python 2 and Python 3

PR-URL: #1836
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: João Reis <[email protected]>
Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

RSLGTM, confirmed this fixes one of the Windows errors.

@joaocgreis
Copy link
Member

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/150/

Landed in 5459eca.

@cclauss I've squashed both commits, please let me know if I made a mistake. Thanks!

@joaocgreis joaocgreis closed this Jul 23, 2019
@cclauss cclauss deleted the ninja.py-StringIO branch July 23, 2019 17:28
@rvagg rvagg mentioned this pull request Sep 26, 2019
rvagg pushed a commit that referenced this pull request Sep 26, 2019
This change supports access to StringIO on both Python 2 and Python 3

PR-URL: #1836
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: João Reis <[email protected]>
@rvagg rvagg mentioned this pull request Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants