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

vm: name anonymous functions #9388

Closed
wants to merge 2 commits into from
Closed

vm: name anonymous functions #9388

wants to merge 2 commits into from

Conversation

solebox
Copy link
Contributor

@solebox solebox commented Oct 31, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

vm

Description of change

named arrow function handlers :)

Ref: #8913

@nodejs-github-bot nodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Oct 31, 2016
@@ -18,19 +18,21 @@ const realRunInContext = Script.prototype.runInContext;

Script.prototype.runInThisContext = function(options) {
if (options && options.breakOnSigint) {
return sigintHandlersWrap(() => {
const sigintHandler = () => {
Copy link
Member

Choose a reason for hiding this comment

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

If line length is below 80 chars I think it is ok to put the whole function expression on one line.

Copy link
Member

Choose a reason for hiding this comment

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

I am a bit 😕 about the name here… this function does not handle any kind of signal; it’s the exact opposite, the function is run undisturbed by SIGINTs. (The comment/code for sigintHandlersWrap has a bit more information.) So maybe you could call it realRunInThisContext or runScriptUninterruped or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where is the doc you are referring to , i want to understand this a little better

@benjamingr
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/5938/console

Going to land if no one objects.

benjamingr pushed a commit that referenced this pull request Nov 6, 2016
Name anonymous arrow function in vm module to improve readability

PR-URL: #9388
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <inglor@gmail.com>
Ref: #8913
@benjamingr
Copy link
Member

Landed in 5079763

Thanks!

@benjamingr benjamingr closed this Nov 6, 2016
evanlucas pushed a commit that referenced this pull request Nov 7, 2016
Name anonymous arrow function in vm module to improve readability

PR-URL: #9388
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <inglor@gmail.com>
Ref: #8913
@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2017
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

#10816 should land at the same time as this.

@MylesBorins MylesBorins added dont-land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants