eslintrc,lib,test: enable prefer-const rule #3152

thefourtheye · 2015-10-01T20:29:09Z

Description from: http://eslint.org/docs/rules/prefer-const.html

If a variable is never modified, using the const declaration is
better. const declaration tells readers, "this variable is never
modified," reducing cognitive load and improving maintainability.

Refer: #3118

cjihrig · 2015-10-01T20:35:02Z

LGTM

ChALkeR · 2015-10-01T21:20:05Z

LGTM if tests pass.

silverwind · 2015-10-03T08:38:17Z

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

silverwind · 2015-10-03T11:56:58Z

CI is green except an unrelated issue which I'm handling in #3167.

thefourtheye · 2015-10-06T13:30:53Z

@silverwind LGTY?

silverwind · 2015-10-06T16:18:57Z

test/parallel/test-async-wrap-check-providers.js

    key: fs.readFileSync(common.fixturesDir + '/keys/ec-key.pem'),
    cert: fs.readFileSync(common.fixturesDir + '/keys/ec-cert.pem')
  };
-  let server = tls.createServer(options, noop).listen(common.PORT, function() {
+  const server = tls.createServer(options).listen(common.PORT, function() {


We sure that removing this listener won't change behaviour?

@silverwind I don't think it will affect the behaviour as noop has no executable statements, so in this particular case, it is as good as not attaching.

Well, I'd be careful nonetheless. Some parts (which I can't rembember) of our API do behave different when you pass undefined vs a noop. I'd rather leave that there unless we can be absolutely sure.

@silverwind I agree. I changed it now. PTAL.

silverwind · 2015-10-06T16:19:20Z

LGTM with a question.

silverwind · 2015-10-06T17:30:59Z

LGTM now 😉

thefourtheye · 2015-10-06T17:34:31Z

I am thinking about splitting the commit into two, one to enable the rule and the other to make the changes. Sounds okay?

silverwind · 2015-10-06T17:36:24Z

Fine with me, we did that on the previous rule additions too.

thefourtheye · 2015-10-08T18:05:43Z

People, few other commits landed after this PR was created and they also violate this rule. So, I updated the files and force pushed. PTAL and give go/no-go.

silverwind · 2015-10-08T18:11:19Z

.eslintrc

@@ -83,6 +83,9 @@ rules:
  # Custom rules in tools/eslint-rules
  require-buffer: 2

+  # Suggest using `const` wherever possible
+  prefer-const: 2
+


Oh, btw: We try to section rules like on this page: http://eslint.org/docs/rules/. Could you move it above the custom rules and add modify it like this:

# ECMAScript 6 # list: http://eslint.org/docs/rules/#ecmascript-6 ## Suggest using 'const' wherever possible prefer-const: 2

@silverwind This has been taken care in the latest force push. Thanks :-)

thefourtheye · 2015-10-25T18:51:59Z

Is it okay to land this people? cc @nodejs/collaborators

Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

cjihrig · 2015-10-25T20:04:00Z

Isn't there still an issue with the REPL and this change?

thefourtheye · 2015-10-26T00:19:08Z

@cjihrig Files in lib are run in strict mode only. So, the linter rule will not affect them. (I mean that the values in loop will be bound to the const variable on each iteration). REPL has this problem only in sloppy mode and it is explained in #3152 (comment)

cjihrig · 2015-10-26T00:26:46Z

Right, I get that. I guess my question is, will any Node APIs fail in the REPL after this change? If certain user entered code is already failing in the REPL in sloppy mode, and is unrelated to this change, then it's fine.

thefourtheye · 2015-10-26T00:36:56Z

will any Node APIs fail in the REPL after this change

Except lib/punycode.js, all the other files have module level strict mode. So, they will not fail.

cjihrig · 2015-10-26T00:43:21Z

I'm good with it then.

thefourtheye · 2015-10-27T02:51:55Z

@Trott @silverwind @targos LGTY guys?

targos · 2015-10-27T05:28:09Z

LGTM if CI is still happy

thefourtheye · 2015-10-27T12:06:43Z

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

Fishrock123 · 2015-10-27T13:13:13Z

"eslintrc" isn't a valid subsystem :) "tools" is probably better?

silverwind · 2015-10-27T17:08:48Z

LGTM. Those CI results seem to get harder to navigate every day. 😭

Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

thefourtheye · 2015-10-27T17:42:42Z

Thanks for the review people. Changes landed at aaf9b48 and b0e7b36.

@Fishrock123 The commit message's title has tools only. I think the PR title only was wrong :(

Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

jasnell · 2015-12-17T19:03:01Z

Landing in LTS primarily to ensure easier cherry picking of commits later

As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins · 2015-12-29T21:22:30Z

Landed in v4.x-staging as 930b483 and 3127666

Had to remove changes to tests that do not live in v4.x

As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

mscdex added the meta Issues and PRs related to the general management of the project. label Oct 6, 2015

silverwind reviewed Oct 6, 2015
View reviewed changes

thefourtheye force-pushed the enable-prefer-const-eslint-rule branch from 30a0cd6 to 75d1381 Compare October 6, 2015 16:52

thefourtheye force-pushed the enable-prefer-const-eslint-rule branch from 75d1381 to 1dedb84 Compare October 8, 2015 18:03

silverwind reviewed Oct 8, 2015
View reviewed changes

thefourtheye force-pushed the enable-prefer-const-eslint-rule branch from 1dedb84 to a9fa79f Compare October 8, 2015 18:31

thefourtheye mentioned this pull request Oct 8, 2015

prefer-const barks at looping variables eslint/eslint#4088

Closed

thefourtheye added 2 commits October 26, 2015 00:23

thefourtheye force-pushed the enable-prefer-const-eslint-rule branch from a9fa79f to 06d5aee Compare October 25, 2015 19:07

thefourtheye closed this Oct 27, 2015

thefourtheye deleted the enable-prefer-const-eslint-rule branch October 27, 2015 17:42

rvagg mentioned this pull request Oct 28, 2015

Preparing v4.2.2 LTS Maintenance nodejs/Release#50

Closed

rvagg mentioned this pull request Oct 29, 2015

Release proposal: v5.0.0 #3466

Merged

nzakas mentioned this pull request Nov 4, 2015

Change Request: prefer-const to not recommend const loop variable eslint/eslint#4333

Closed

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

jasnell added the lts-watch-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eslintrc,lib,test: enable prefer-const rule #3152

eslintrc,lib,test: enable prefer-const rule #3152

thefourtheye commented Oct 1, 2015

cjihrig commented Oct 1, 2015

ChALkeR commented Oct 1, 2015

silverwind commented Oct 3, 2015

silverwind commented Oct 3, 2015

thefourtheye commented Oct 6, 2015

silverwind Oct 6, 2015

thefourtheye Oct 6, 2015

silverwind Oct 6, 2015

thefourtheye Oct 6, 2015

silverwind commented Oct 6, 2015

silverwind commented Oct 6, 2015

thefourtheye commented Oct 6, 2015

silverwind commented Oct 6, 2015

thefourtheye commented Oct 8, 2015

silverwind Oct 8, 2015

thefourtheye Oct 8, 2015

thefourtheye commented Oct 25, 2015

cjihrig commented Oct 25, 2015

thefourtheye commented Oct 26, 2015

cjihrig commented Oct 26, 2015

thefourtheye commented Oct 26, 2015

cjihrig commented Oct 26, 2015

thefourtheye commented Oct 27, 2015

targos commented Oct 27, 2015

thefourtheye commented Oct 27, 2015

Fishrock123 commented Oct 27, 2015

silverwind commented Oct 27, 2015

thefourtheye commented Oct 27, 2015

jasnell commented Dec 17, 2015

MylesBorins commented Dec 29, 2015

eslintrc,lib,test: enable prefer-const rule #3152

eslintrc,lib,test: enable prefer-const rule #3152

Conversation

thefourtheye commented Oct 1, 2015

cjihrig commented Oct 1, 2015

ChALkeR commented Oct 1, 2015

silverwind commented Oct 3, 2015

silverwind commented Oct 3, 2015

thefourtheye commented Oct 6, 2015

silverwind Oct 6, 2015

Choose a reason for hiding this comment

thefourtheye Oct 6, 2015

Choose a reason for hiding this comment

silverwind Oct 6, 2015

Choose a reason for hiding this comment

thefourtheye Oct 6, 2015

Choose a reason for hiding this comment

silverwind commented Oct 6, 2015

silverwind commented Oct 6, 2015

thefourtheye commented Oct 6, 2015

silverwind commented Oct 6, 2015

thefourtheye commented Oct 8, 2015

silverwind Oct 8, 2015

Choose a reason for hiding this comment

thefourtheye Oct 8, 2015

Choose a reason for hiding this comment

thefourtheye commented Oct 25, 2015

cjihrig commented Oct 25, 2015

thefourtheye commented Oct 26, 2015

cjihrig commented Oct 26, 2015

thefourtheye commented Oct 26, 2015

cjihrig commented Oct 26, 2015

thefourtheye commented Oct 27, 2015

targos commented Oct 27, 2015

thefourtheye commented Oct 27, 2015

Fishrock123 commented Oct 27, 2015

silverwind commented Oct 27, 2015

thefourtheye commented Oct 27, 2015

jasnell commented Dec 17, 2015

MylesBorins commented Dec 29, 2015