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

repl: don't override all internal repl defaults by cjihrig · Pull Request #7826 · nodejs/node · GitHub

/ node Public

repl: don't override all internal repl defaults - #7826

Merged
cjihrig merged 2 commits into
nodejs:masterfrom
cjihrig:globals
Aug 8, 2016
ConversationCommits (2)ChecksFiles changed
Merged

repl: don't override all internal repl defaults#7826
cjihrig merged 2 commits into
nodejs:masterfrom
cjihrig:globals

Conversation

cjihrig commented Jul 21, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

The createInternalRepl() method accepts an options object as an argument. However, if one is provided, it overrides all of the default options. This PR applies the options object to the defaults, only changing the values that are explicitly set.

The approach in this PR also adds a method to test/common.js, allowing specific global variables to be whitelisted. This is because the CLI REPL sets useGlobal to true, and then defines module and require() as globals. Alternative approaches could be to set useGlobal to false (which was what was silently happening before) or set common.globalCheck to false. I prefer the current approach because it is more representative of what happens in the real world, and still performs the global checks.

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 21, 2016

Fishrock123 commented Jul 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Sounds good to me.

We could probably get rid of turning off global checking in favor of always using the new API?

Edit, CI: https://ci.nodejs.org/job/node-test-pull-request/3374/

cjihrig commented Jul 22, 2016

Copy link
Copy Markdown
Contributor Author

We could probably get rid of turning off global checking in favor of always using the new API?

Yep, that's the plan assuming this lands, and nothing too crazy is in the other tests.

cjihrig commented Jul 22, 2016

Copy link
Copy Markdown
Contributor Author

CI was all 💚

cjihrig commented Aug 1, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@Fishrock123 can your "Sounds good to me." be taken as an official LGTM?

cjihrig commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell could I trouble you for a review?

jasnell commented Aug 5, 2016

Copy link
Copy Markdown
Member

LGTM!

cjihrig commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

Thanks, James! CI, since it's been a while: https://ci.nodejs.org/job/node-test-pull-request/3541/

cjihrig added 2 commits August 8, 2016 11:03
test: allow globals to be whitelisted
f18b1c9 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <jasnell@gmail.com>
repl: don't override all internal repl defaults
2d4a521 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig merged commit 2d4a521 into nodejs:master Aug 8, 2016
cjihrig deleted the globals branch August 8, 2016 15:04
cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig added a commit that referenced this pull request Aug 10, 2016
test: allow globals to be whitelisted
9735acc 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: #7826
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit that referenced this pull request Aug 10, 2016
repl: don't override all internal repl defaults
61e57e0 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: #7826
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig mentioned this pull request Aug 11, 2016
Merged
MylesBorins modified the milestones: v4.6.2, v4.7.0 Oct 24, 2016

MylesBorins commented Nov 18, 2016

Copy link
Copy Markdown
Contributor

ping @cjihrig

cjihrig added a commit to cjihrig/node that referenced this pull request Nov 23, 2016
test: allow globals to be whitelisted
fd97cf0 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit to cjihrig/node that referenced this pull request Nov 23, 2016
repl: don't override all internal repl defaults
9a0d9fc 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig mentioned this pull request Nov 23, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
test: allow globals to be whitelisted
02e8187 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: #7826
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
repl: don't override all internal repl defaults
698bf2e 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: #7826
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
test: allow globals to be whitelisted
2fd27a4 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: #7826
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
repl: don't override all internal repl defaults
13c4a6b 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: #7826
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Dec 21, 2016
Merged
MylesBorins added a commit that referenced this pull request Jan 3, 2017
2017-01-03, Version 4.7.1 'Argon' (LTS)
1035318 
This LTS release comes with 180 commits. This includes 117 which are
test related, 34 which are doc related, 15 which are build / tool
related, and 1 commit which is an update to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* repl:
  - Passing options to the repl will no longer overwrite defaults
    (cjihrig) #7826
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10395
MylesBorins added a commit that referenced this pull request Jan 4, 2017
2017-01-03, Version 4.7.1 'Argon' (LTS)
b26a469 
This LTS release comes with 180 commits. This includes 117 which are
test related, 34 which are doc related, 15 which are build / tool
related, and 1 commit which is an update to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* repl:
  - Passing options to the repl will no longer overwrite defaults
    (cjihrig) #7826
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10395
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
2017-01-03, Version 4.7.1 'Argon' (LTS)
774acef 
    This LTS release comes with 180 commits. This includes 117 which are
    test related, 34 which are doc related, 15 which are build / tool
    related, and 1 commit which is an update to dependencies.

    Notable Changes:

    * build:
      - shared library support is now working for AIX builds
        (Stewart Addison) nodejs/node#9675
    * repl:
      - Passing options to the repl will no longer overwrite defaults
        (cjihrig) nodejs/node#7826
    * timers:
      - Re canceling a cancelled timers will no longer throw
        (Jeremiah Senkpiel) nodejs/node#9685

    PR-URL: nodejs/node#10395

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
2017-01-03, Version 4.7.1 'Argon' (LTS)
13f539e 
    This LTS release comes with 180 commits. This includes 117 which are
    test related, 34 which are doc related, 15 which are build / tool
    related, and 1 commit which is an update to dependencies.

    Notable Changes:

    * build:
      - shared library support is now working for AIX builds
        (Stewart Addison) nodejs/node#9675
    * repl:
      - Passing options to the repl will no longer overwrite defaults
        (cjihrig) nodejs/node#7826
    * timers:
      - Re canceling a cancelled timers will no longer throw
        (Jeremiah Senkpiel) nodejs/node#9685

    PR-URL: nodejs/node#10395

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
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

No reviews

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL