Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

thefourtheye
Copy link
Contributor

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

cjihrig commented Oct 1, 2015

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented Oct 1, 2015

LGTM if tests pass.

@silverwind
Copy link
Contributor

@silverwind
Copy link
Contributor

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

@thefourtheye
Copy link
Contributor Author

@silverwind LGTY?

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Oct 6, 2015
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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@silverwind
Copy link
Contributor

LGTM with a question.

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

LGTM now 😉

@thefourtheye
Copy link
Contributor Author

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

@silverwind
Copy link
Contributor

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

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

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.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@thefourtheye
Copy link
Contributor Author

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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@thefourtheye thefourtheye force-pushed the enable-prefer-const-eslint-rule branch from a9fa79f to 06d5aee Compare October 25, 2015 19:07
@cjihrig
Copy link
Contributor

cjihrig commented Oct 25, 2015

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

@thefourtheye
Copy link
Contributor Author

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

cjihrig commented Oct 26, 2015

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

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

cjihrig commented Oct 26, 2015

I'm good with it then.

@thefourtheye
Copy link
Contributor Author

@Trott @silverwind @targos LGTY guys?

@targos
Copy link
Member

targos commented Oct 27, 2015

LGTM if CI is still happy

@thefourtheye
Copy link
Contributor Author

@Fishrock123
Copy link
Contributor

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

@silverwind
Copy link
Contributor

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

thefourtheye added a commit that referenced this pull request Oct 27, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
thefourtheye added a commit that referenced this pull request Oct 27, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@thefourtheye
Copy link
Contributor Author

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 :(

@thefourtheye thefourtheye deleted the enable-prefer-const-eslint-rule branch October 27, 2015 17:42
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@rvagg rvagg mentioned this pull request Oct 29, 2015
@rvagg rvagg mentioned this pull request Dec 17, 2015
@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

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

thefourtheye added a commit that referenced this pull request Dec 29, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
thefourtheye added a commit that referenced this pull request Dec 29, 2015
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@MylesBorins
Copy link
Contributor

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

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

MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
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 <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.