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

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

Closed
wants to merge 1 commit into from

Conversation

jkzing
Copy link
Contributor

@jkzing jkzing commented Jul 16, 2017

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

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

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

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

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

@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

@Trott
Copy link
Member

Trott commented Jul 18, 2017

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

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.

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.

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
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
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
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
@addaleax addaleax mentioned this pull request Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.