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

[v6.x] addons: remove semicolons from after module definition #13879

Conversation

gabrielschulhof
Copy link
Contributor

Having semicolons there runs counter to our documentation and illicits
warnings in pedantic mode. This removes semicolons from after uses of
NODE_MODULE and NODE_MODULE_CONTEXT_AWARE_BUILTIN.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v6.x vm Issues and PRs related to the vm subsystem. labels Jun 22, 2017
@gabrielschulhof
Copy link
Contributor Author

Re #12919
@MylesBorins there's no NODE_MODULE_CONTEXT_AWARE_BUILTIN in src/node.cc in v6.x.

@gabrielschulhof
Copy link
Contributor Author

bump

@addaleax addaleax changed the base branch from v6.x to v6.x-staging July 6, 2017 16:22
@addaleax addaleax changed the title addons: remove semicolons from after module definition [v6.x] addons: remove semicolons from after module definition Jul 6, 2017
@addaleax
Copy link
Member

addaleax commented Jul 6, 2017

@gabrielschulhof Usually, these PRs are handled by the person managing the next release – maybe @gibfahn wants to merge this right now, but most of the time there’s no need to bump, it will get sorted out in time. :)

Also, just so you know, backports should target the staging branches if possible (that doesn’t seem to be a problem here), and it’s nice if you edit the PR description in some way so people don’t think it’s a new PR when glancing over it. :)

@gabrielschulhof
Copy link
Contributor Author

@addaleax thanks for the pointers! I'll keep these in mind for next time :)

@MylesBorins
Copy link
Contributor

@gabrielschulhof which PR is this a backport of? We should have the original commit meta data including all sign offs in the commit message

@gabrielschulhof
Copy link
Contributor Author

@MylesBorins sorry I didn't follow SOP on this.

2767209 and #12919

@gabrielschulhof
Copy link
Contributor Author

@MylesBorins should I update the commit message?

@MylesBorins
Copy link
Contributor

@gabrielschulhof indeed

the message should be

addons: remove semicolons from after module definition
Having semicolons there runs counter to our documentation and illicits
warnings in pedantic mode. This removes semicolons from after uses of
NODE_MODULE and NODE_MODULE_CONTEXT_AWARE_BUILTIN.

PR-URL: #12919
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>

We usually then add a Backport-PR-URL: https://github.com/nodejs/node/pull/13879 when landing

Having semicolons there runs counter to our documentation and illicits
warnings in pedantic mode. This removes semicolons from after uses of
NODE_MODULE and NODE_MODULE_CONTEXT_AWARE_BUILTIN.

PR-URL: nodejs#12919
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
@gabrielschulhof gabrielschulhof force-pushed the remove-semicolons-v6.x branch from 6704f23 to 9429373 Compare July 6, 2017 17:00
@gabrielschulhof
Copy link
Contributor Author

@MylesBorins done. Thanks @MylesBorins and @addaleax for the guidance!

@gibfahn gibfahn self-assigned this Jul 6, 2017
@MylesBorins
Copy link
Contributor

landed in 8850007

@gabrielschulhof gabrielschulhof deleted the remove-semicolons-v6.x branch July 19, 2017 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants