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

url: replace var with let in lib/url.js #30281

Closed
wants to merge 1 commit into from
Closed

url: replace var with let in lib/url.js #30281

wants to merge 1 commit into from

Conversation

xefimx
Copy link
Contributor

@xefimx xefimx commented Nov 6, 2019

url: replace var with let and const in lib/url.js

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

@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Nov 6, 2019
@ChALkeR ChALkeR added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2019
trivikr
trivikr approved these changes Nov 6, 2019
View reviewed changes
lib/url.js
let end = -1;
let rest = '';
let lastPos = 0;
let i = 0;
Copy link
Member

Choose a reason for hiding this comment

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

nit: move i = 0 inside the loop on line 166

Also, add let in a for loop on line 298 and 414?

for (let i = 0; i < rest.length; ++i) {

lib/url.js
var hostEnd = -1;
var atSign = -1;
var nonHost = -1;
let hostEnd = -1;
Copy link
Member

Choose a reason for hiding this comment

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

nit: the variable hostEnd can move inside the loop

@gireeshpunathil
Copy link
Member

@trivikr - are your suggestions a blocker from landing? plmk.

@nodejs-github-bot
Copy link
Collaborator

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

@trivikr
Copy link
Member

trivikr commented Nov 12, 2019

@trivikr - are your suggestions a blocker from landing? plmk.

@gireeshpunathil Nope, they're not a blocker.
I've given approval and prefixed nit to my suggestions.

It's optional for @xefimx to follow-up if they want.

@trivikr trivikr mentioned this pull request Nov 13, 2019
Closed
2 tasks
Trott pushed a commit that referenced this pull request Nov 17, 2019
@xefimx @Trott
url: replace var with let in lib/url.js
87cef76 
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 17, 2019

Landed in 87cef76

@Trott Trott closed this Nov 17, 2019
@Trott
Copy link
Member

Trott commented Nov 17, 2019

Thanks for the contribution! 🎉

@trivikr trivikr mentioned this pull request Nov 17, 2019
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
@xefimx @MylesBorins
url: replace var with let in lib/url.js
3b45f8f 
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
Merged
targos pushed a commit that referenced this pull request Dec 1, 2019
@xefimx @targos
url: replace var with let in lib/url.js
1413377 
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
trivikr added a commit to trivikr/node that referenced this pull request Dec 7, 2019
@trivikr
url: declare iterator inside loop
92ee25c 
Refs: nodejs#30281 (comment)
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
Closed
bmeck pushed a commit that referenced this pull request Dec 11, 2019
@trivikr @bfarias-godaddy
url: declare iterator inside loop
9c32b24 
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
@trivikr @MylesBorins
url: declare iterator inside loop
5ab3ca4 
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@xefimx @MylesBorins
url: replace var with let in lib/url.js
f2f4529 
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@trivikr @targos
url: declare iterator inside loop
70dc7a2 
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@trivikr @BethGriggs
url: declare iterator inside loop
6d674d4 
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@xefimx @gireeshpunathil @nodejs-github-bot @trivikr @Trott @BridgeAR @ChALkeR