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.

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

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

Merged
merged 1 commit into from
May 18, 2019
Merged

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

merged 1 commit into from
May 18, 2019

Conversation

refack
Copy link
Contributor

@refack refack 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-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 15, 2019
@refack refack 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
tools/js2c.py Show resolved Hide resolved
tools/js2c.py Show resolved Hide resolved
tools/js2c.py Show resolved Hide resolved
tools/js2c.py Outdated Show resolved Hide resolved
tools/js2c.py Outdated Show resolved Hide resolved
tools/js2c.py Outdated Show resolved Hide resolved
tools/js2c.py Outdated Show resolved Hide resolved
tools/js2c.py Outdated Show resolved Hide resolved
tools/js2c.py Outdated Show resolved Hide resolved
tools/js2c.py Outdated Show resolved Hide resolved
@cclauss cclauss mentioned this pull request Jan 29, 2019
Closed
63 tasks
cclauss
cclauss reviewed Feb 2, 2019
View reviewed changes
tools/js2c.py Outdated Show resolved Hide resolved
@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]

@refack refack mentioned this pull request Mar 19, 2019
Closed
@refack refack force-pushed the tweak-js2c-2 branch 4 times, most recently from 1e23681 to 14912d8 Compare April 4, 2019 23:03
@refack refack 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
Contributor Author

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)
return config
Copy link
Contributor

@cclauss cclauss Apr 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 Booleans
return config.replace('\'', '"').replace('"true"', 'true').replace('"false"', 'false')

cclauss
cclauss approved these changes Apr 5, 2019
View reviewed changes
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 work!

@refack refack self-assigned this Apr 5, 2019
@refack
Copy link
Contributor Author

refack commented Apr 12, 2019

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

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

This requires a rebase.

@refack refack force-pushed the tweak-js2c-2 branch 3 times, most recently from 2c3d1e7 to 2627fea Compare April 15, 2019 22:22
@nodejs nodejs deleted a comment from nodejs-github-bot Apr 15, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Apr 15, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs nodejs deleted a comment from nodejs-github-bot Apr 15, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Apr 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.

@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

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

@nodejs-github-bot
Copy link
Collaborator

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

@refack refack merged commit bfbc035 into nodejs:master May 18, 2019
@refack refack 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]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
@joyeecheung
Copy link
Member

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
Contributor Author

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.

@refack refack mentioned this pull request May 30, 2019
Closed
3 tasks
@refack
Copy link
Contributor Author

refack commented May 30, 2019

Regression fix in #27980

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

@joyeecheung joyeecheung joyeecheung left review comments

@thefourtheye thefourtheye thefourtheye left review comments

@cclauss cclauss cclauss approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. python PRs 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