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

url: change scoping of variables with let #4867

Closed

Conversation

kthelgason
Copy link
Contributor

This PR addresses variable redeclarations in lib/url.js by changing hoisted vars to let.
Also changes some vars to const as recommended by the linter.

@Trott points out in #4853 that the linter reports a lot of redeclarations. I believe addressing this incrementally is easier and less risky, hence this PR only fixes these issues in this one file.

Also changes some `var`s to `const` as they never change.
@tflanagan
Copy link
Contributor

I'm confused as to what made you change var to let or const, you are changing some but not all. I understand the changes in the for loops, but it seems you are leaking beyond that scope.

IE: Line 674, authInHost

@kthelgason
Copy link
Contributor Author

I tried to handle all cases where a variable was redeclared. The for loop scopes were obvious, but there were also some other cases, like the one you mention, authInHost. I could have been more clear on that in the description.

@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label Jan 25, 2016
@Trott
Copy link
Member

Trott commented Jan 25, 2016

@Trott
Copy link
Member

Trott commented Jan 25, 2016

LGTM if CI is OK

@Trott
Copy link
Member

Trott commented Jan 25, 2016

@tflanagan If it's useful, here's the eslint report that @kthelgason was probably looking at to decide which instances in the file to change. https://gist.github.com/Trott/7dc16adf550b6a8d06cc

@kthelgason
Copy link
Contributor Author

@Trott @tflanagan yes that is indeed the report I was looking at.
The CI fails building on Ubuntu15.04, any idea what might be wrong?

@Trott
Copy link
Member

Trott commented Jan 26, 2016

@kthelgason It looks like a problem with the CI host itself and not anything in your code. Will run CI again so we can hopefully see green on Ubuntu 15.04 and declare victory.

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

@Trott
Copy link
Member

Trott commented Jan 26, 2016

CI is green. LGTM

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

LGTM

Trott pushed a commit to Trott/io.js that referenced this pull request Jan 27, 2016
Also changes some `var`s to `const` as they never change.

PR-URL: nodejs#4867
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Jan 27, 2016

Landed in fcae05e

@Trott Trott closed this Jan 27, 2016
rvagg pushed a commit that referenced this pull request Jan 28, 2016
Also changes some `var`s to `const` as they never change.

PR-URL: #4867
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
Also changes some `var`s to `const` as they never change.

PR-URL: #4867
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
Also changes some `var`s to `const` as they never change.

PR-URL: #4867
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Also changes some `var`s to `const` as they never change.

PR-URL: #4867
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Also changes some `var`s to `const` as they never change.

PR-URL: #4867
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Also changes some `var`s to `const` as they never change.

PR-URL: nodejs#4867
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants