FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

vm: name anonymous functions by solebox · Pull Request #9388 · nodejs/node · GitHub

/ node Public

vm: name anonymous functions - #9388

Closed
solebox wants to merge 2 commits into
nodejs:masterfrom
solebox:vm
ConversationCommits (2)ChecksFiles changed
Closed

vm: name anonymous functions#9388
solebox wants to merge 2 commits into
nodejs:masterfrom
solebox:vm

Conversation

solebox commented Oct 31, 2016 •
edited by Fishrock123
Loading

Copy link
Copy Markdown
Contributor
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 added the vm Issues and PRs related to the vm subsystem. label Oct 31, 2016
lpinca reviewed Nov 1, 2016
View reviewed changes
Comment thread lib/vm.js Outdated
Script.prototype.runInThisContext = function(options) {
if (options && options.breakOnSigint) {
return sigintHandlersWrap(() => {
const sigintHandler = () => {

lpinca Nov 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Nov 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

solebox Nov 1, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

benjamingr commented Nov 6, 2016

Copy link
Copy Markdown
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
vm: name anonymous functions
5079763 
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 commented Nov 6, 2016

Copy link
Copy Markdown
Member

Landed in 5079763

Thanks!

benjamingr closed this Nov 6, 2016
evanlucas pushed a commit that referenced this pull request Nov 7, 2016
vm: name anonymous functions
b083086 
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
evanlucas mentioned this pull request Nov 7, 2016
Merged
MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2017
Trott mentioned this pull request May 15, 2017
Closed
3 tasks

gibfahn commented Jun 17, 2017

Copy link
Copy Markdown
Member

#10816 should land at the same time as this.

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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
lpinca lpinca left review comments
jasnell jasnell approved these changes

Assignees

No one assigned

Labels

vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL