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

test: replace string concatenation with path.join in test-graph.tls-write.js #14272

Closed
wants to merge 1 commit into from
Closed

test: replace string concatenation with path.join in test-graph.tls-write.js #14272

wants to merge 1 commit into from

Conversation

jkzing
Copy link
Contributor

@jkzing jkzing commented Jul 16, 2017 •
edited
Loading

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

test

This is a PR from JSConf CN Code & Learn workshop. 👻

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Jul 16, 2017
@jkzing jkzing changed the title [Code & Learn] test: use template string literals in test-graph.tls-write.js test: use template string literals in test-graph.tls-write.js Jul 16, 2017
@joyeecheung joyeecheung added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 16, 2017
Trott
Trott approved these changes Jul 16, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@TimothyGu TimothyGu added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. and removed code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. labels Jul 16, 2017
@vsemozhetbyt
Copy link
Contributor

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

refack
refack suggested changes Jul 16, 2017
View reviewed changes
test/async-hooks/test-graph.tls-write.js Outdated
@@ -20,8 +20,8 @@ hooks.enable();
//
const server = tls
.createServer({
cert: fs.readFileSync(common.fixturesDir + '/test_cert.pem'),
key: fs.readFileSync(common.fixturesDir + '/test_key.pem')
cert: fs.readFileSync(`${common.fixturesDir}/test_cert.pem`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to throw a wrench, and this should be path.join

Copy link
Member

@tniessen tniessen Jul 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We have had the same discussion in other PRs before, and we would have to change this in quite many files. It does not make it worse than it is :)

But I agree that we should use path.join, so if you want to change this... @jkzing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem on that.
So if I touch string concatenation that used as path next time, path.join is preferred rather than template string syntax, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO is these cases path.join is much better:

  1. it's more correct — Windows uses \ while other OSs use /, and path.join handles that
  2. it's more expressive — you explicitly say "this is a path I'm working with here"
  3. more readable — a little bit easier to see the parts that are being joined

refack
refack approved these changes Jul 16, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯
Thanks you!

@refack
Copy link
Contributor

refack commented Jul 16, 2017

@jkzing thank you very much for you contribution. "Change requests" are a normal part of the process. Personally I'm very happy you did follow up and made the code even better 🥇 Hope to see you contributing more.

@tniessen tniessen self-assigned this Jul 16, 2017
@jkzing
Copy link
Contributor Author

jkzing commented Jul 16, 2017 •
edited
Loading

@refack With pleasure. Code review makes our code better.😄

This was referenced Jul 16, 2017
Closed
Closed
Closed
Closed
@Trott
Copy link
Member

Trott commented Jul 16, 2017

Looks like something went wrong with a number of CI builds. We'll have to re-run this on CI, but unfortunately not right now, because it's got quite a large backlog of jobs at the moment.

@Trott
Copy link
Member

Trott commented Jul 17, 2017

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

@Trott
Copy link
Member

Trott commented Jul 18, 2017

Two test failures in CI are unrelated. CI can be considered green!

trevnorris
trevnorris suggested changes Jul 18, 2017
View reviewed changes
Copy link
Contributor

@trevnorris trevnorris left a comment •
edited
Loading

Choose a reason for hiding this comment

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

git title is too long. Please limit it to 50 characters. You can remove the file name. That's easy enough to see by looking at the commit stat.

@jkzing jkzing changed the title test: use template string literals in test-graph.tls-write.js test: replace string concatenation with path.join in test-graph.tls-write.js Jul 18, 2017
@jkzing
Copy link
Contributor Author

jkzing commented Jul 18, 2017

@trevnorris done.

trevnorris
trevnorris approved these changes Jul 18, 2017
View reviewed changes
Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

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

Thanks much

refack pushed a commit to refack/node that referenced this pull request Jul 19, 2017
@jkzing @refack
test: replace string concatenation with path.join
97008a7 
PR-URL: nodejs#14272
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@refack
Copy link
Contributor

refack commented Jul 19, 2017

Landed in 97008a7

@refack refack closed this Jul 19, 2017
@jkzing jkzing deleted the replace-concat branch July 20, 2017 03:01
addaleax pushed a commit that referenced this pull request Jul 22, 2017
@jkzing @addaleax
test: replace string concatenation with path.join
8d28d08 
PR-URL: #14272
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
@jkzing @Fishrock123
test: replace string concatenation with path.join
94c7331 
PR-URL: #14272
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@addaleax addaleax mentioned this pull request Jul 24, 2017
Closed
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@tniessen tniessen tniessen approved these changes

@trevnorris trevnorris trevnorris approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees

@tniessen tniessen

Labels
async_hooks Issues and PRs related to the async hooks subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.