test: replace string concatenation with template string literals in test-fs-watchfile.js#14287
Uh oh!
There was an error while loading. Please reload this page.
Conversation
joyeecheung added the code-and-learn 
joyeecheung commented Jul 16, 2017
test/parallel/test-fs-watchfile.js Outdated
| // Omitting AIX. It works but not reliably. | ||
| if(common.isLinux||common.isOSX||common.isWindows){ | ||
| constdir=common.tmpDir+'/watch'; | ||
| constdir=`${common.tmpDir}/watch'`; |
There was a problem hiding this comment.
You forgot to remove the last quote.
Trott left a comment
Trott left a comment There was a problem hiding this comment.
Will LGTM once that last quotation mark is removed. :-D
Helianthus21 commented Jul 16, 2017
Sorry for my careless. |
Trott left a comment
Trott left a comment There was a problem hiding this comment.
LGTM if CI is green. Thank you for the contribution!
benjamingr left a comment
benjamingr left a comment There was a problem hiding this comment.
Thanks for your contribution and welcome to the project :)
There was a problem hiding this comment.
I don't want to block this seeing the number of approvals, but this should really use path.join instead, see #14272 (comment)
test/parallel/test-fs-watchfile.js Outdated
| // Omitting AIX. It works but not reliably. | ||
| if(common.isLinux||common.isOSX||common.isWindows){ | ||
| constdir=common.tmpDir+'/watch'; | ||
| constdir=`${common.tmpDir}/watch`; |
There was a problem hiding this comment.
This is Ok.
But it would be much better to use path.join instead, see #14272 (comment)
vsemozhetbyt commented Jul 16, 2017
Trott commented Jul 17, 2017
CI is green. I'm +0 on using Look for lots of "converting template literals to |
blade254353074 commented Jul 17, 2017
@Trott Can you change the code? or only @Helianthus21 can do it? |
tniessen commented Jul 17, 2017
@blade254353074 We recommend to always enable "Allow edits from maintainers" which in fact allows us to make changes on our own. However, we usually do not do that without explicit permission of the PR author. In this case, replacing the template with |
Helianthus21 commented Jul 18, 2017
Hey guys, I have already used |
PR-URL: #14287 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Jul 19, 2017
Landed with a few nits fixed in 6a587ad. |
PR-URL: #14287 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #14287 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
15 participants
[JSConf CN Code&Learn]
Replace string concatenation in test/parallel/test-fs-watchfile.js with template literals.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes