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

test: scope redeclared variable by Trott · Pull Request #4854 · nodejs/node · GitHub

/ node Public

test: scope redeclared variable - #4854

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:no-redeclare-test-assert
ConversationCommits (1)ChecksFiles changed
Closed

test: scope redeclared variable#4854
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:no-redeclare-test-assert

Conversation

Trott commented Jan 25, 2016

Copy link
Copy Markdown
Member

test-assert.js redeclares a variable with var. This change converts
it to a const declaration and wraps it in an IIFE to scope it to just
the test that uses it.

Trott added assert Issues and PRs related to the assert subsystem. test Issues and PRs related to the tests. labels Jan 25, 2016

Trott commented Jan 25, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/1368/

cjihrig commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

LGTM

One thing we might want to consider while moving forward with block scoping is that we don't need to use IIFEs. The changes in this PR could just be wrapped in curly braces and create a new block scope.

silverwind commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

@cjihrig do you know since when v8 supports the curly scope thing? Is it listed on the ES6 table?

targos commented Jan 25, 2016

Copy link
Copy Markdown
Member

@silverwind It is already supported in strict mode.

silverwind commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

Oh, I never knew, thanks!

cjihrig commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

@silverwind I think it's just part of the definition of block scope. It works at least back to io.js v1.0.0.

silverwind commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

So called standalone blocks seem to have been around since JavaScript 1.0 according to MDN. It's the combination of strict mode creating a scope on them and block scoped vars that makes them useful now.

test: scope redeclared variable
73bb5bb 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.
Trott force-pushed the no-redeclare-test-assert branch from 7cb700f to 73bb5bb Compare January 25, 2016 19:19

Trott commented Jan 25, 2016

Copy link
Copy Markdown
Member Author

I sure like the idea of not having the creation of a function as unnecessary overhead. Converted to block scope, force pushed, looks good.

CI: https://ci.nodejs.org/job/node-test-pull-request/1373/

silverwind commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

targos commented Jan 25, 2016

Copy link
Copy Markdown
Member

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Jan 26, 2016
test: scope redeclared variable
34daaa7 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: nodejs#4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>

Trott commented Jan 26, 2016

Copy link
Copy Markdown
Member Author

Landed in 34daaa7

Trott closed this Jan 26, 2016
rvagg pushed a commit that referenced this pull request Jan 27, 2016
test: scope redeclared variable
195ed3a 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: #4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
benjamingr pushed a commit to benjamingr/io.js that referenced this pull request Jan 27, 2016
test: scope redeclared variable
e99a50d 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: nodejs#4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>

jasnell commented Jan 27, 2016

Copy link
Copy Markdown
Member

Added the lts-watch-v4.x label. For commits like this, getting them on the lts-watch list for v4.x helps significantly when determining which commits need to be cherry picked. We cherry pick almost all the test commits unless they specifically relate to semver-minor/major updates.

rvagg pushed a commit that referenced this pull request Feb 8, 2016
test: scope redeclared variable
6539c64 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: #4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
test: scope redeclared variable
9e84e22 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: #4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
test: scope redeclared variable
80a637a 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: #4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
test: scope redeclared variable
5dd05be 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: #4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
test: scope redeclared variable
4093963 
`test-assert.js` redeclares a variable with `var`. This change converts
it to a `const` declaration and wraps it in a standalone block to scope
it to just the test that uses it.

PR-URL: nodejs#4854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Trott deleted the no-redeclare-test-assert branch January 13, 2022 22:32
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

assert Issues and PRs related to the assert subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL