Skip to content

tools: refactor js2c.py for maximal python3 compat#25518

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.

Sign up for GitHub

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

Jump to bottom
Merged
refack merged 1 commit into from
May 18, 2019
Merged

tools: refactor js2c.py for maximal python3 compat #25518

refack merged 1 commit into from
May 18, 2019

Conversation

@refack
Copy link
Contributor

@refackrefack commented Jan 15, 2019•
edited
Loading

  • Code before and after this PR generates the exact same output.
  • Tested with python2 (2.6 & 2.7) and python3 (3.6 & 3.7)
  • Passes make lint-py PYTHON=python2 and make lint-py PYTHON=python3
  • Whitespace changes are to comply with PEP8

/CC @nodejs/python

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 15, 2019
@refackrefack added the python PRs and issues that require attention from people who are familiar with Python. label Jan 15, 2019
thefourtheye
thefourtheye reviewed Jan 16, 2019
View reviewed changes
@cclausscclauss mentioned this pull request Jan 29, 2019
Closed
63 tasks
cclauss
cclauss reviewed Feb 2, 2019
View reviewed changes
@cclauss
Copy link
Contributor

cclauss commented Feb 2, 2019•
edited
Loading

Fascinating what upstream did... v8/v8@7164251

@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019•
edited
Loading

@nodejs/python @nodejs/tooling @nodejs/build PTAL

@cclauss
Copy link
Contributor

cclauss commented Mar 6, 2019

Linter errors...

src/node_native_module.h:25: Lines should be <= 80 characters long [whitespace/line_length] [2] src/node_native_module.h:96: Lines should be <= 80 characters long [whitespace/line_length] [2]

@refackrefack mentioned this pull request Mar 19, 2019
Closed
@refackrefackforce-pushed the tweak-js2c-2 branch 4 times, most recently from 1e23681 to 14912d8CompareApril 4, 2019 23:03
@refackrefack mentioned this pull request Apr 4, 2019
Closed
3 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@refack
Copy link
ContributorAuthor

refack commented Apr 4, 2019

@nodejs/python PTAL I've addressed most of the comments, and cleaned this up some more.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
cclauss
cclauss reviewed Apr 5, 2019
View reviewed changes
tools/js2c.py
# 2. turn pseudo-booleans strings into Booleans
config=re.sub('"true"', 'true', config)
config=re.sub('"false"', 'false', config)
returnconfig
Copy link
Contributor

@cclausscclaussApr 5, 2019•
edited
Loading

Choose a reason for hiding this comment

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

# four lines --> one...# normalize string literals from ' into " and turn pseudo-booleans strings into Booleansreturnconfig.replace('\'', '"').replace('"true"', 'true').replace('"false"', 'false')

cclauss
cclauss approved these changes Apr 5, 2019
View reviewed changes
Copy link
Contributor

@cclausscclauss left a comment

Choose a reason for hiding this comment

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

Great work!

@refackrefack self-assigned this Apr 5, 2019
@refack
Copy link
ContributorAuthor

refack commented Apr 12, 2019

@nodejs/python @nodejs/build-files PTAL.

@refackrefack mentioned this pull request Apr 15, 2019
Closed
4 tasks
@BridgeAR
Copy link
Member

BridgeAR commented Apr 15, 2019

This requires a rebase.

@refackrefackforce-pushed the tweak-js2c-2 branch 3 times, most recently from 2c3d1e7 to 2627feaCompareApril 15, 2019 22:22
@nodejsnodejs deleted a comment from nodejs-github-botApr 15, 2019
@nodejsnodejs deleted a comment from nodejs-github-botApr 15, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejsnodejs deleted a comment from nodejs-github-botApr 15, 2019
@nodejsnodejs deleted a comment from nodejs-github-botApr 15, 2019
@Trott
Copy link
Member

Trott commented Apr 23, 2019

This looks good to me but my Python expertise is limited. Would much prefer it land with a review from @nodejs/python, but I'll give it a somewhat-rubber stamp LGTM if no one more informed steps in to review/approve/oppose/whatever it.

joyeecheung
joyeecheung reviewed Apr 23, 2019
View reviewed changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@refack
tools: refactor js2c.py for maximal Python3 compatibility
bfbc035
* add explicit `--target` argument to enable succinct gyp declaration * simplify js2c semantics PR-URL: nodejs#25518 Reviewed-By: Christian Clauss <[email protected]>
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 18, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23188/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 18, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23189/

@refackrefack merged commit bfbc035 into nodejs:masterMay 18, 2019
@refackrefack deleted the tweak-js2c-2 branch May 18, 2019 18:00
targos pushed a commit that referenced this pull request May 20, 2019
@refack@targos
tools: refactor js2c.py for maximal Python3 compatibility
0db846f
* add explicit `--target` argument to enable succinct gyp declaration * simplify js2c semantics PR-URL: #25518 Reviewed-By: Christian Clauss <[email protected]>
@BridgeARBridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
@joyeecheung
Copy link
Member

joyeecheung commented May 29, 2019

This landed as tools: refactor js2c.py for maximal python3 compat even though it contains changes that make every lib source uint16_t which is not required for python 3. compat, and it's not mentioned at all in the commit message..

@refack
Copy link
ContributorAuthor

refack commented May 30, 2019

@joyeecheung you are right. That was not the intention of this PR and it's probably a rebasing artifact. I intended to move all the one-bye/two-byte changes to #27095 (we're we agreed to shelf that change)...
I'll post a correction ASAP.

@refackrefack mentioned this pull request May 30, 2019
Closed
3 tasks
@refack
Copy link
ContributorAuthor

refack commented May 30, 2019

Regression fix in #27980

@refackrefack removed their assignment Jun 6, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheungjoyeecheungjoyeecheung left review comments

@cclausscclausscclauss approved these changes

+1 more reviewer

@thefourtheyethefourtheyethefourtheye left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buildIssues and PRs related to build files or the CI.pythonPRs and issues that require attention from people who are familiar with Python.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@refack@nodejs-github-bot@cclauss@BridgeAR@Trott@joyeecheung@thefourtheye