Skip to content

Conversation

@jason9693
Copy link
Contributor

@jason9693jason9693 commented Sep 22, 2017

I think using var name 'objUrl' is more clear then 'u'.

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

url

I think using var name 'objUrl' is better then 'u'...
@nodejs-github-botnodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Sep 22, 2017
Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

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

I would prefer urlObject which is already used in some other places in that file, but otherwise LGTM.

@mscdex
Copy link
Contributor

I also think urlObject would be better.

@jason9693
Copy link
ContributorAuthor

jason9693 commented Sep 22, 2017

Oh, I see. I'll change it.

Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM.

@tniessen
Copy link
Member

Landed in 7d55b81, thank you for your contribution! 🎉

tniessen pushed a commit that referenced this pull request Sep 26, 2017
PR-URL: #15551 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
PR-URL: #15551 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
PR-URL: #15551 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
PR-URL: nodejs/node#15551 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
PR-URL: #15551 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
PR-URL: #15551 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
PR-URL: #15551 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Nov 3, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

urlIssues and PRs related to the legacy built-in url module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@jason9693@mscdex@tniessen@jasnell@lpinca@TimothyGu@cjihrig@BridgeAR@MylesBorins@nodejs-github-bot