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

lib: replace string concatenation with template #16923

Closed
wants to merge 1 commit into from
Closed

lib: replace string concatenation with template #16923

wants to merge 1 commit into from

Conversation

VijiKannan
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

This is my first contribution to node project

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Nov 10, 2017
@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 10, 2017
cjihrig
cjihrig approved these changes Nov 10, 2017
View reviewed changes
lib/_http_client.js Outdated
@@ -116,7 +116,7 @@ function ClientRequest(options, cb) {

var path;
if (options.path) {
path = '' + options.path;
path = `${options.path}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this could just be String(options.path).

Copy link
Member

@Trott Trott Nov 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@cjihrig '' + foo seems pretty idiomatic at this point, perhaps due to performance. I'm not sure if that's a case of CrankshaftScript though.
¯\(ツ)/¯

Copy link
Contributor

Choose a reason for hiding this comment

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

'' + foo is what is being removed here though.

gireeshpunathil
gireeshpunathil reviewed Nov 11, 2017
View reviewed changes
Copy link
Member

@gireeshpunathil gireeshpunathil left a comment

Choose a reason for hiding this comment

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

PR author and committer are not the same - probably worked on the same system with clobbered git config parameters. @Trott - please advise how to rectify it.

@fhinkel
Copy link
Member

fhinkel commented Nov 11, 2017

@gireeshpunathil We can change the author when we land this. Just let me know what the correct name and email address are. @VijiKannan if you prefer, you could also try to do the same change from a system where git config user.name and git config user.email are set to the correct person. Then we could just close this PR. Let us know what's easiest for you. Thanks!

@VijiKannan VijiKannan force-pushed the nodepr branch 3 times, most recently from df5ece3 to 17fec4b Compare November 14, 2017 09:46
@VijiKannan
Copy link
Contributor Author

I have addressed the comments, and also used the correct git config. Please have a look, thanks!

@apapirovski
Copy link
Member

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

@gireeshpunathil
Copy link
Member

Inconclusive CI results (failed in smartos and raspbian weezy), trying once again: https://ci.nodejs.org/job/node-test-pull-request/11496/

@tniessen
Copy link
Member

Landed in a0aff57, thank you for your contribution! 🎉

@tniessen tniessen closed this Nov 17, 2017
tniessen pushed a commit that referenced this pull request Nov 17, 2017
@VijiKannan @tniessen
lib: replace string concatenation with template
a0aff57 
PR-URL: #16923
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@VijiKannan @MylesBorins
lib: replace string concatenation with template
1a8b0e9 
PR-URL: #16923
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@VijiKannan @gibfahn
lib: replace string concatenation with template
7737d11 
PR-URL: #16923
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@VijiKannan @gibfahn
lib: replace string concatenation with template
5d06a4f 
PR-URL: #16923
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil left review comments

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott left review comments

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski 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. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.