tools,test: var to const by BridgeAR · Pull Request #13732 · nodejs/node

BridgeAR · 2017-06-17T00:53:15Z

As far as I know using const is preferable in almost all situations, so this converts probably all var declarations to const, if possible.

If this is not what you want, I'm happy to close this again or split into smaller parts.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools,test

mscdex · 2017-06-17T01:05:06Z

Even though we're TurboFan by default now, I would still be cautious about permanent deopts when using const/let. You might want to at least run a make test-check-deopts | grep -ivP ' <(?:\/|SharedFunctionInfo)' and see what aborted/disabled optimizations shows up. Some are expected though, such as the deopts in domain and vm.

BridgeAR · 2017-06-17T01:58:20Z

@mscdex I did that and there was no new constant deopt.

mscdex · 2017-06-17T03:03:19Z

Actually I just checked current master and it looks like make test-check-deopts is broken again :-(

Typically you will see a lot of output with that command I gave, because tests tend to have a lot of deopts (although the grep ignores the content of those deopt messages it still prints the filenames).

EDIT: It looks like I was mistaken. I'm guessing I just haven't ran that since the switch to V8 5.9 and TurboFan by default. There's only about 6 perm deopts as of this writing and I believe they're all inside the tests themselves.

vsemozhetbyt · 2017-06-17T10:01:52Z

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

gibfahn · 2017-06-18T08:51:06Z

@gibfahn the prefer-const rule only changes let, not var.
I'll move the lib and benchmark parts out as you requested.

Yes, but we also have the no-var rule for tests.

vsemozhetbyt · 2017-06-18T09:48:10Z

@gibfahn We had // eslint-disable-next-line no-var in that single place in test.

vsemozhetbyt · 2017-06-18T09:49:20Z

test/common/index.js

It seems this line can be removed now.

cjihrig

LGTM with @vsemozhetbyt's comment addressed.

jasnell · 2017-06-19T15:44:45Z

While the code changes look good, I'd much rather the changes to each of the separate tools be in their own separate commits. On the off chance that we need to revert any particular item, it would make it significantly easier.

refack · 2017-06-19T15:50:26Z

While the code changes look good, I'd much rather the changes to each of the separate tools be in their own separate commits. On the off chance that we need to revert any particular item, it would make it significantly easier.

@BridgeAR If I understand @jasnell correctly separate commits not PRs (not like we did for /libs/. Just saying...)

BridgeAR · 2017-06-19T16:01:11Z

Comments addressed. I'm not to sure about the commit messages though.

refack · 2017-06-19T16:07:14Z

Comments addressed. I'm not to sure about the commit messages though.

Personally I would do tools: TOOLNAME - var to const
But let's wait for further feedback.

cjihrig · 2017-06-19T16:12:31Z

Maybe even tools: change var to const in TOOLNAME (as long as it fits in 50 characters).

refack · 2017-06-19T16:14:30Z

tools/license2rtf.js

Suggestion
const {li, in_license_block: lic} = paragraph;

refack · 2017-06-19T16:15:24Z

tools/license2rtf.js

const line_indent = Math.floor(line_strip_length / 2) * 2;

refack · 2017-06-19T16:17:16Z

tools/eslint-rules/required-modules.js

suggestion:
module.exports = function({report, options:requiredModules})

refack · 2017-06-19T16:20:50Z

About my suggestions, if we're churning, let's make some butter (or code better).
But wait for more feedback.

BridgeAR · 2017-06-23T21:57:04Z

PTAL

I'd rebase and address the comments after getting some more feedback.

BridgeAR · 2017-07-02T21:39:44Z

Rebased and comments addressed

refack · 2017-07-02T22:05:44Z

pre-land CI: https://ci.nodejs.org/job/node-test-commit/10891/

refack · 2017-07-02T22:10:00Z

tools/eslint-rules/required-modules.js

something here fails the linter:

Running JS linter... ./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \ benchmark doc lib test tools /usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/rule-context.js:210 this.eslint.report( ^ TypeError: Cannot read property 'eslint' of undefined at report (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/rule-context.js:210:13) at /usr/home/iojs/build/workspace/node-test-linter/tools/eslint-rules/required-modules.js:79:11 at Array.forEach (native) at Linter.Program:exit (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint-rules/required-modules.js:78:24) at emitOne (events.js:101:20) at Linter.emit (events.js:188:7) at NodeEventGenerator.applySelector (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/util/node-event-generator.js:265:26) at NodeEventGenerator.applySelectors (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/util/node-event-generator.js:294:22) at NodeEventGenerator.leaveNode (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/util/node-event-generator.js:317:14) at CodePathAnalyzer.leaveNode (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/code-path-analysis/code-path-analyzer.js:622:23) at Traverser.leave (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/linter.js:925:36) gmake: *** [Makefile:876: jslint-ci] Error 2 /usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/rule-context.js:210 this.eslint.report( ^

https://ci.nodejs.org/job/node-test-linter/10203/console

refack · 2017-07-02T22:25:29Z

New CI: https://ci.nodejs.org/job/node-test-commit/10892/

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-07-02T23:29:31Z

Landed in 017122a a5347c9 ea67c27 a7e1cee 461049f 0808989 bce27c2

refack · 2017-07-02T23:32:14Z

@nodejs/build this PR touches some of the tools that are not covered by the tests, anyway to verify they work as expected also where the are needed?

joaocgreis · 2017-07-04T14:21:57Z

@refack If I'm not missing anything, this only influences make doc and and the licence displayed by the MSI (since common and eslint run in CI). https://nodejs.org/download/nightly/v9.0.0-nightly20170703fe730d34ce/ already has this change, the docs folder and the MSI seem to have been correctly generated.

refack · 2017-07-04T15:09:09Z

@refack If I'm not missing anything, this only influences make doc and and the licence displayed by the MSI (since common and eslint run in CI). https://nodejs.org/download/nightly/v9.0.0-nightly20170703fe730d34ce/ already has this change, the docs folder and the MSI seem to have been correctly generated.

Thanks!

vsemozhetbyt added the refactor to ES6+ label Jun 17, 2017

vsemozhetbyt reviewed Jun 18, 2017

View reviewed changes

test/common/index.js Outdated

Copy link

Contributor

vsemozhetbyt Jun 18, 2017

Choose a reason for hiding this comment

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

It seems this line can be removed now.

cjihrig approved these changes Jun 18, 2017

View reviewed changes

refack approved these changes Jun 19, 2017

View reviewed changes

XadillaX mentioned this pull request Jun 19, 2017

dns: make var to const if possible #13797

Closed

2 tasks

refack suggested changes Jul 2, 2017

View reviewed changes

refack approved these changes Jul 2, 2017

View reviewed changes

BridgeAR added 7 commits July 2, 2017 19:26

tools: change var to const in ./doc/addon-verify

bce27c2

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

tools: change var to const in ./doc/html

0808989

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

tools: change var to const in ./doc/json

461049f

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

tools: change var to const in ./doc/preprocess

a7e1cee

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

tools: change var to const in ./eslint-rules

ea67c27

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

tools: change var to const in ./license2rtf

a5347c9

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

test: change var to const in ./common

017122a

PR-URL: nodejs#13732 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

addaleax mentioned this pull request Jul 3, 2017

v8.2.0 proposal #13744

Merged

Uh oh!

Conversation

BridgeAR commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Jun 17, 2017

Uh oh!

BridgeAR commented Jun 17, 2017

Uh oh!

mscdex commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Jun 17, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

vsemozhetbyt commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt Jun 18, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 19, 2017

Uh oh!

refack commented Jun 19, 2017

Uh oh!

BridgeAR commented Jun 19, 2017

Uh oh!

refack commented Jun 19, 2017

Uh oh!

cjihrig commented Jun 19, 2017

Uh oh!

refack Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 19, 2017

Uh oh!

BridgeAR commented Jun 23, 2017

Uh oh!

BridgeAR commented Jul 2, 2017

Uh oh!

refack commented Jul 2, 2017

Uh oh!

refack Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 2, 2017

Uh oh!

refack commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 2, 2017

Uh oh!

joaocgreis commented Jul 4, 2017

Uh oh!

refack commented Jul 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

BridgeAR commented Jun 17, 2017 •

edited

Loading

mscdex commented Jun 17, 2017 •

edited

Loading

vsemozhetbyt commented Jun 18, 2017 •

edited

Loading

refack commented Jul 2, 2017 •

edited

Loading