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: replace var with let in lib/url.js #30281

Closed
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
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) {

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

@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.

Trott pushed a commit that referenced this pull request Nov 17, 2019
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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
2 tasks
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
targos pushed a commit that referenced this pull request Dec 1, 2019
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
trivikr added a commit to trivikr/node that referenced this pull request Dec 7, 2019
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
bmeck pushed a commit that referenced this pull request Dec 11, 2019
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
PR-URL: #30281
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Refs: #30281 (comment)

PR-URL: #30509
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[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
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
Development

Successfully merging this pull request may close these issues.

7 participants