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

repl: declare for initial expression in the for loop, instead of hoisting and re-using #29535

Closed
wants to merge 2 commits into from

Conversation

lholmquist
Copy link
Contributor

There was a for loop that had an initial Expression that wasn't using let/var, this adds a let to it

I also noticed that this module uses var instead of let/const. I saw in another issue,#28009 (comment) , the reasoning for why they haven't been updated yet. But since i am touching this file, I would like to try to do the conversion for this file. If someone else agrees i can push another commit with those updates, if not, then i'll just keep this PR as is

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

* Adds a  to a variable declaration in a for loop
  that wasn't using anything.
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Sep 12, 2019
@mscdex
Copy link
Contributor

mscdex commented Sep 12, 2019

The loop is using a previously declared i variable (from line 1092).

@lholmquist
Copy link
Contributor Author

lholmquist commented Sep 12, 2019

@mscdex ah, i see it. however, it is not consistent with how the other loops are doing things. And it is also going across functions which seems wrong. I also realize this code base dates back a while, but i think we can start to clean things up like this, when hoisting all your variables to the top of the function was the thing to do

perhaps the title of the issue could change to better reflect what is being done?

I also saw a couple more for loops where that variable was being used. i'm going to update the PR with those also.

i missed a couple in the previous commit.  this also removes the hoisted variable that was being used for those loops

This can be squashed
@lholmquist lholmquist changed the title repl: add missing variable declaration in for loop repl: declare for initial expression in the for loop, instead of hoisting and re-using Sep 12, 2019
@lholmquist
Copy link
Contributor Author

@addaleax @trivikr i would also like to update all the vars to let/const since i'm already in this file touching things. should i just send that as a separate PR, so this one doesn't get to cluttered?

@addaleax
Copy link
Member

@lholmquist Personally, I think a separate commit is fine, given that that reduces the likelihood of merge conflicts with this PR. Others may feel differently, though.

@nodejs-github-bot
Copy link
Collaborator

@lholmquist
Copy link
Contributor Author

@addaleax thanks for the tip. I might just wait until this is merged, then send another PR(referencing this one), just so it doesn't clutter this PR to much

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 14, 2019

Landed in 30e919a

@Trott Trott closed this Sep 14, 2019
Trott pushed a commit that referenced this pull request Sep 14, 2019
* Adds `let` to a variable declaration in a for loop
  that wasn't using anything.

* Declare the for initial expression in the for loop.

* Remove hoisted variables for loops.

PR-URL: #29535
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
lholmquist added a commit to lholmquist/node that referenced this pull request Sep 16, 2019
* This goes along with the PR, nodejs#29535, that started to
clean up the repl module.

This PR replaces the instances of var with let/const.
Trott pushed a commit that referenced this pull request Sep 19, 2019
Refs: #29535

This PR replaces the instances of var with let/const.

PR-URL: #29575
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2019
* Adds `let` to a variable declaration in a for loop
  that wasn't using anything.

* Declare the for initial expression in the for loop.

* Remove hoisted variables for loops.

PR-URL: #29535
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2019
Refs: #29535

This PR replaces the instances of var with let/const.

PR-URL: #29575
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
* Adds `let` to a variable declaration in a for loop
  that wasn't using anything.

* Declare the for initial expression in the for loop.

* Remove hoisted variables for loops.

PR-URL: #29535
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
Refs: #29535

This PR replaces the instances of var with let/const.

PR-URL: #29575
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants