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: change ContextifyScript to Script in comment #8415

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@danbev
Member

danbev commented Sep 6, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

vm

Description of change

Reading the comment at the top of the vm.js, I think that
ContextifyScript should perhaps just be Script.

vm: changing ContextifyScript to Script in comment
Reading the comment at the top of the vm.js, I think that
ContextifyScript should perhaps just be Script.
@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Sep 6, 2016

Contributor

LGTM

Contributor

princejwesley commented Sep 6, 2016

LGTM

@mscdex mscdex added the vm label Sep 6, 2016

Show outdated Hide outdated lib/vm.js
@@ -4,7 +4,7 @@ const binding = process.binding('contextify');
const Script = binding.ContextifyScript;
// The binding provides a few useful primitives:
// - ContextifyScript(code, { filename = "evalmachine.anonymous",
// - Script(code, { filename = "evalmachine.anonymous",
// displayErrors = true } = {})

This comment has been minimized.

@thefourtheye

thefourtheye Sep 6, 2016

Contributor

Nit: I think this line was aligned with the previous line's filename.

@thefourtheye

thefourtheye Sep 6, 2016

Contributor

Nit: I think this line was aligned with the previous line's filename.

This comment has been minimized.

@mscdex

mscdex Sep 6, 2016

Contributor

Yep, need to reduce the indentation

@mscdex

mscdex Sep 6, 2016

Contributor

Yep, need to reduce the indentation

This comment has been minimized.

@danbev

danbev Sep 6, 2016

Member

Ah thanks, I'll fix that.

@danbev

danbev Sep 6, 2016

Member

Ah thanks, I'll fix that.

@mscdex mscdex referenced this pull request Sep 6, 2016

Closed

Bot failed to label PR #71

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 6, 2016

Contributor

LGTM

Contributor

cjihrig commented Sep 6, 2016

LGTM

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 6, 2016

Member

LGTM

Member

fhinkel commented Sep 6, 2016

LGTM

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Sep 6, 2016

Contributor

LGTM

Contributor

thefourtheye commented Sep 6, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 7, 2016

Member

LGTM

Member

jasnell commented Sep 7, 2016

LGTM

@danbev danbev changed the title from vm: changing ContextifyScript to Script in comment to vm: change ContextifyScript to Script in comment Sep 7, 2016

fhinkel added a commit that referenced this pull request Sep 8, 2016

vm: change ContextifyScript to Script in comment
Reading the comment at the top of the vm.js, I think that
ContextifyScript should perhaps just be Script.

PR-URL: #8415
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franzih@chromium.org>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 8, 2016

Member

Landed in da0651a

Member

fhinkel commented Sep 8, 2016

Landed in da0651a

@fhinkel fhinkel closed this Sep 8, 2016

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Sep 8, 2016

Member

@fhinkel Thanks for merging this!

Member

danbev commented Sep 8, 2016

@fhinkel Thanks for merging this!

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 8, 2016

Member

Sure. Thanks for contributing!

Member

fhinkel commented Sep 8, 2016

Sure. Thanks for contributing!

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

vm: change ContextifyScript to Script in comment
Reading the comment at the top of the vm.js, I think that
ContextifyScript should perhaps just be Script.

PR-URL: #8415
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franzih@chromium.org>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@danbev danbev deleted the danbev:vm.js-contextify-comment branch Sep 26, 2016

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

should this be backported to v4?

Member

MylesBorins commented Sep 30, 2016

should this be backported to v4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment