Skip to content

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

Merged
cjihrig merged 2 commits intonodejs:masterfrom
cjihrig:globals
Aug 8, 2016
Merged

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

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 21, 2016

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 nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 21, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 21, 2016 •
edited
Loading

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
Copy link
Contributor Author

cjihrig commented Jul 22, 2016

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
Copy link
Contributor Author

cjihrig commented Jul 22, 2016

CI was all 💚

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 1, 2016 •
edited
Loading

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 5, 2016

@jasnell could I trouble you for a review?

@jasnell
Copy link
Member

jasnell commented Aug 5, 2016

LGTM!

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 5, 2016

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
@cjihrig
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>
@cjihrig
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 cjihrig merged commit 2d4a521 into nodejs:master Aug 8, 2016
@cjihrig cjihrig deleted the globals branch August 8, 2016 15:04
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig added a commit that referenced this pull request Aug 10, 2016
@cjihrig
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
@cjihrig
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 cjihrig mentioned this pull request Aug 11, 2016
Merged
@MylesBorins MylesBorins modified the milestones: v4.6.2, v4.7.0 Oct 24, 2016
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 18, 2016

ping @cjihrig

cjihrig added a commit to cjihrig/node that referenced this pull request Nov 23, 2016
@cjihrig
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
@cjihrig
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 cjihrig mentioned this pull request Nov 23, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
@cjihrig @MylesBorins
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
@cjihrig @MylesBorins
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
@cjihrig @MylesBorins
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
@cjihrig @MylesBorins
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 MylesBorins mentioned this pull request Dec 21, 2016
Merged
MylesBorins added a commit that referenced this pull request Jan 3, 2017
@MylesBorins
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
@MylesBorins
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
@imyller
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
@imyller
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>
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

@cjihrig @Fishrock123 @jasnell @MylesBorins @nodejs-github-bot