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: rm py semicolons (Python != JavaScript) #29120

Closed
wants to merge 1 commit into from
Closed

build: rm py semicolons (Python != JavaScript) #29120

wants to merge 1 commit into from

Conversation

MattIPv4
Copy link
Member

Follow-up PR from #29105 (review) that applies Flake8 E703 (no semicolons) to all Python files.

Checklist

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Aug 14, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Can you undo the changes to deps/v8/ and tools/gyp/? Those are vendored in and not managed by this repository.

@MattIPv4
Copy link
Member Author

Ah, do gyp changes need to occur in node-gyp?
Where do v8 changes need to be made?

@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Aug 14, 2019
@Trott
Copy link
Member

Trott commented Aug 14, 2019

Ah, do gyp changes need to occur in node-gyp?

I'm not sure to be honest. /ping @nodejs/node-gyp

Where do v8 changes need to be made?

Contributing to V8 is described at https://v8.dev/docs/contribute

@sam-github
Copy link
Contributor

There is a chance that if @cclauss pushes a v8 change here, @targos can push it upstream into v8.

@cclauss
Copy link
Contributor

cclauss commented Aug 14, 2019

For obvious reasons, I like this PR but... based @addaleax comment above this might not be the right forum.

  1. The vast majority of the effected files are v8 which does not accept PRs at https://github.com/v8/v8 so landing takes effort that @Trott links to and @targos knows well.
  2. The gyp files could be covered by a PR to https://github.com/nodejs/node-gyp.
  3. build: py3 configure.py #29106 Already contains these changes for configure.py.
  4. That leaves just one semicolon in tools/icu/shrink-icu-src.py that this PR could directly effect.

@MattIPv4
Copy link
Member Author

@targos May you assist with getting these changes made to v8?

@Trott
Copy link
Member

Trott commented Aug 16, 2019

There was a conflict in deps/v8/tools/gen-postmortem-metadata.py, so I rebased and force-pushed.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 16, 2019

  1. That leaves just one semicolon in tools/icu/shrink-icu-src.py that this PR could directly effect.

Reworked the commit here (preserving authorship) and opened #29170.

Going to close this, since it shouldn't land, but feel free to continue discussion about getting the changes upstreamed to V8. If you think it should be open for any reason, go ahead and re-open (or if GitHub won't allow that, post a comment asking that it be re-opened).

@Trott Trott closed this Aug 16, 2019
@MattIPv4
Copy link
Member Author

Sounds good. Hopefully, someone can help land the v8 changes 👍

@rvagg
Copy link
Member

rvagg commented Aug 20, 2019

sorry, just for clarity I'll address:

Ah, do gyp changes need to occur in node-gyp?

I'm not sure to be honest. /ping @nodejs/node-gyp

They are not in sync, a PR here should be followed by a PR to node-gyp, ideally, and vice versa. We're still trying to figure out how to maintain this mess.

@MattIPv4
Copy link
Member Author

@rvagg As I've got the PR open in node-gyp already, should I create a separate one in node that duplicates those changes?

@rvagg
Copy link
Member

rvagg commented Aug 20, 2019

@MattIPv4 yep, I think so, if they're changes for tools/gyp then it's probably worth doing, we're out of sync already and don't have a process to get back into sync yet so doing things manually isn't going to hurt for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants