Skip to content

src: change var name 'u' in url.js line 97 #15551

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
Closed
jason9693 wants to merge 2 commits into from
Closed

src: change var name 'u' in url.js line 97 #15551

jason9693 wants to merge 2 commits into from

Conversation

@jason9693
Copy link
Contributor

@jason9693 jason9693 commented Sep 22, 2017 •
edited by tniessen
Loading

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

@jason9693
change var name 'u' in line 97
e8feb30 
I think using var name 'objUrl' is better then 'u'...
@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Sep 22, 2017
TimothyGu
TimothyGu approved these changes Sep 22, 2017
View reviewed changes
Copy link
Member

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

mscdex commented Sep 22, 2017

I also think urlObject would be better.

@jason9693
Copy link
Contributor Author

jason9693 commented Sep 22, 2017 •
edited
Loading

Oh, I see. I'll change it.

TimothyGu
TimothyGu approved these changes Sep 22, 2017
View reviewed changes
Copy link
Member

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

tniessen commented Sep 26, 2017

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

@tniessen tniessen closed this Sep 26, 2017
tniessen pushed a commit that referenced this pull request Sep 26, 2017
@jason9693 @tniessen
url: change variable name to be more descriptive
7d55b81 
PR-URL: #15551
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
@jason9693 @MylesBorins
url: change variable name to be more descriptive
1c4c4f9 
PR-URL: #15551
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
@jason9693 @MylesBorins
url: change variable name to be more descriptive
5992a31 
PR-URL: #15551
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
@jason9693 @addaleax
url: change variable name to be more descriptive
ed53097 
PR-URL: nodejs/node#15551
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
@jason9693 @MylesBorins
url: change variable name to be more descriptive
34b4180 
PR-URL: #15551
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
@jason9693 @MylesBorins
url: change variable name to be more descriptive
892d224 
PR-URL: #15551
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@jason9693 @MylesBorins
url: change variable name to be more descriptive
27af0bb 
PR-URL: #15551
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

10 participants

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