test: replace string concatenation with path.join in test-graph.tls-write.js#14272
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added async_hooks 
jkzing changed the title 
joyeecheung added the code-and-learn 
TimothyGu added code-and-learn 
vsemozhetbyt commented Jul 16, 2017
There was a problem hiding this comment.
Sorry to throw a wrench, and this should be path.join
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
IMHO is these cases path.join is much better:
- it's more correct — Windows uses
\while other OSs use/, andpath.joinhandles that - it's more expressive — you explicitly say "this is a path I'm working with here"
- more readable — a little bit easier to see the parts that are being joined
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. |
@refack With pleasure. Code review makes our code better.😄 |
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 commented Jul 17, 2017
Trott commented Jul 18, 2017
Two test failures in CI are unrelated. CI can be considered green! |
jkzing changed the title jkzing commented Jul 18, 2017
@trevnorris done. |
PR-URL: nodejs#14272 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
refack commented Jul 19, 2017
Landed in 97008a7 |
PR-URL: #14272 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
PR-URL: #14272 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
14 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test
This is a PR from JSConf CN Code & Learn workshop. 👻