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

Enable block scoped var eslint rule #3151

Conversation

thefourtheye
Copy link
Contributor

Description from: http://eslint.org/docs/rules/block-scoped-var.html

The block-scoped-var rule generates warnings when variables are used
outside of the block in which they were defined. This emulates C-style
block scope.

Refer: #3118

@thefourtheye thefourtheye force-pushed the enable-block-scoped-var-eslint-rule branch from 55fd8c1 to ccf1c16 Compare October 1, 2015 20:15
@cjihrig
Copy link
Contributor

cjihrig commented Oct 1, 2015

A lot of these changes (but not all) seem unnecessary to me. A mass conversion to let was already -1'ed, and var isn't block scoped.

Not 100% sure about this, but I think I read somewhere that it's better to initialize your variables.

@@ -21,6 +21,7 @@ tls.createServer({

(function() {
// 2**26 == 64M entries
var junk;
for (var i = 0, junk = [0]; i < 26; ++i) junk = junk.concat(junk);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd initialize it on the var line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Description from: http://eslint.org/docs/rules/block-scoped-var.html

The block-scoped-var rule generates warnings when variables are used
outside of the block in which they were defined. This emulates C-style
block scope.

Refer: nodejs#3118
As per block-scoped-var rule, the variables which were used before
declaration or declared within a block and used outside are trated
as linter errors.

Refer: nodejs#3118
As per block-scoped-var rule, the variables which were used before
declaration or declared within a block and used outside are trated
as linter errors.

Refer: nodejs#3118
@thefourtheye thefourtheye force-pushed the enable-block-scoped-var-eslint-rule branch from ccf1c16 to 615be16 Compare October 4, 2015 14:54
@thefourtheye thefourtheye force-pushed the enable-block-scoped-var-eslint-rule branch from 615be16 to 78a09d6 Compare October 4, 2015 19:04
@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Oct 6, 2015
@Fishrock123
Copy link
Contributor

-1 Just because some people don't like this functionality does not mean it is in any way bad.

@trevnorris
Copy link
Contributor

I'm -0 on this. Personally I dig having my declarations yanked, but as @Fishrock123 said it's not necessarily a bad thing. So not sure it's worth the code churn.

@Qard
Copy link
Member

Qard commented Oct 13, 2015

-1 for code churn.

Why not just gradually change occurrences of var to let and, when there are no more instances of var, add the rule?

@Fishrock123
Copy link
Contributor

I see no good reason to move to let, only const.

var has it's place in everyday usage, why would we change that?

@Qard
Copy link
Member

Qard commented Oct 14, 2015

With let you get less accidental scoping issues causing silent failures rather than throwing reference errors. I don't think it's a huge deal though. It just makes for a slightly nicer upgrade path if you gradually change instances of var to let as you touch particular areas of code. You can use the eventual lack of instances of var as an indicator that everything is now scoped correctly. In a way, let is really just putting scope linting directly into the code.

I don't really care either way on the let thing. It was just a suggestion of a possible path forward, if anyone wanted to go through with it.

@thefourtheye
Copy link
Contributor Author

Closing, as this rule is not preferred generally.

@thefourtheye thefourtheye deleted the enable-block-scoped-var-eslint-rule branch October 16, 2015 18:08
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.

7 participants