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.

Sign up for GitHub

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

Jump to bottom

Create Lint Rule to avoid let in for loops #9045

Closed
MylesBorins opened this issue Oct 12, 2016 · 7 comments
Closed

Create Lint Rule to avoid let in for loops #9045

MylesBorins opened this issue Oct 12, 2016 · 7 comments
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.

Comments

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 12, 2016 •
edited
Loading

In #8873 we removed let from for loops due to potential de-opts. It would be nice to have a linting rule to avoid more instances leaking into the code base.

For context, our other lint rules can be found in tools/eslint-rules
@MylesBorins MylesBorins added good first issue Issues that are suitable for first-time contributors. tools Issues and PRs related to the tools directory. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests. mentor-available labels Oct 12, 2016
@jalafel
Copy link
Contributor

jalafel commented Oct 12, 2016

Hi, I'd like to give this a shot as a contribution!

@MylesBorins
Copy link
Contributor Author

go for it @jessicaquynh

Let me know if you need any help!

@Trott may be a good resource as well, he's written quite a few of the lint rules

@jalafel
Copy link
Contributor

jalafel commented Oct 12, 2016

@thealphanerd will do! Thank you, and I will reach out to @Trott for good resources and paradigms, thanks!

@jalafel jalafel mentioned this issue Oct 12, 2016
Closed
2 tasks
@jalafel
Copy link
Contributor

jalafel commented Oct 12, 2016

I've opened up a PR on this issue for review.

I am unsure if it was necessary, but I included within the lint, definitions for forIn and forOf as well. I would really appreciate input from @thealphanerd and @Trott whenever you have got time! Thanks :)

@jalafel
Copy link
Contributor

jalafel commented Oct 13, 2016

So after making the var changes to the functions in the lib/ directory, I am coming across a redeclare error on the iterator variable ('i' is already defined no-redeclare) in multiple instances.

I am certain it's because of the scope differences between let and var.

I am unsure how to approach resolving this. I could rename all the loop variables, but I don't think that's good convention since some files have more than ten for-loops. Would anyone have any recommended solutions? @thealphanerd @Trott

@Trott
Copy link
Member

Trott commented Oct 13, 2016

@jessicaquynh I don't think there should be any changes at all to the lib directory with this change other than adding one line to .eslintrc. There are no violations of this lint rule in the lib directory so the files there can be left unchanged (other than the .eslintrc file).

@jalafel
Copy link
Contributor

jalafel commented Oct 13, 2016

@Trott Ah! I understand my misstep now. I was running the test with the rule in the node/.eslintrc instead of the one in lib which cropped up those errors. Thank you for the clarification.

jalafel added a commit to jalafel/node that referenced this issue Oct 14, 2016
@jalafel
tools: avoid let in for loops
8b704d6 
This adds a new ESLint tool to check for let
declarations within the for, forIn, forOf expressions.

Fixes: nodejs#9045
Ref: nodejs#8873
@Trott Trott closed this as completed in b16a97e Oct 14, 2016
jasnell pushed a commit that referenced this issue Oct 17, 2016
@jalafel @jasnell
tools: avoid let in for loops
8ac29bd 
This adds a new ESLint tool to check for let
declarations within the for, forIn, forOf expressions.

Fixes: #9045
Ref: #8873
PR-URL: #9049
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 11, 2016
@jalafel
tools: avoid let in for loops
87285ed 
This adds a new ESLint tool to check for let
declarations within the for, forIn, forOf expressions.

Fixes: #9045
Ref: #8873
PR-URL: #9049
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 11, 2016
@jalafel
tools: avoid let in for loops
d1d3379 
This adds a new ESLint tool to check for let
declarations within the for, forIn, forOf expressions.

Fixes: nodejs#9045
Ref: nodejs#9553
Ref: nodejs#8873
PR-URL: nodejs#9049
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 14, 2016
@jalafel
tools: avoid let in for loops
c8dccf2 
This adds a new ESLint tool to check for let
declarations within the for, forIn, forOf expressions.

Fixes: #9045
Ref: #9553
Ref: #8873
PR-URL: #9049
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@princejwesley princejwesley mentioned this issue Nov 20, 2016
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tools: avoid let in for loops jalafel/node
3 participants
@MylesBorins @Trott @jalafel