Skip to content

test: replace string concatenation with template string literals in test-fs-watchfile.js - #14287

Closed
Helianthus21 wants to merge 3 commits into
nodejs:masterfrom
Helianthus21:master
Closed

test: replace string concatenation with template string literals in test-fs-watchfile.js#14287
Helianthus21 wants to merge 3 commits into
nodejs:masterfrom
Helianthus21:master

Conversation

@Helianthus21

@Helianthus21Helianthus21 commented Jul 16, 2017

Copy link
Copy Markdown
Contributor

[JSConf CN Code&Learn]
Replace string concatenation in test/parallel/test-fs-watchfile.js with template literals.

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jul 16, 2017
@joyeecheungjoyeecheung added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 16, 2017
@joyeecheung

joyeecheung commented Jul 16, 2017

Copy link
Copy Markdown
Member

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

targos
targos reviewed Jul 16, 2017
View reviewed changes
Comment threadtest/parallel/test-fs-watchfile.js Outdated
// Omitting AIX. It works but not reliably.
if (common.isLinux || common.isOSX || common.isWindows) {
const dir = common.tmpDir + '/watch';
const dir = `${common.tmpDir}/watch'`;

@targostargosJul 16, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You forgot to remove the last quote.

joyeecheung
joyeecheung approved these changes Jul 16, 2017
View reviewed changes

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

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

Trott
Trott requested changes Jul 16, 2017
View reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will LGTM once that last quotation mark is removed. :-D

@Helianthus21

Helianthus21 commented Jul 16, 2017

Copy link
Copy Markdown
ContributorAuthor

Sorry for my careless.

Trott
Trott approved these changes Jul 16, 2017
View reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

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. Thank you for the contribution!

benjamingr
benjamingr approved these changes Jul 16, 2017
View reviewed changes

@benjamingrbenjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your contribution and welcome to the project :)

@tniessentniessen self-assigned this Jul 16, 2017
tniessen
tniessen reviewed Jul 16, 2017
View reviewed changes

@tniessentniessen left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't want to block this seeing the number of approvals, but this should really use path.join instead, see #14272 (comment)

refack
refack reviewed Jul 16, 2017
View reviewed changes
Comment threadtest/parallel/test-fs-watchfile.js Outdated
// Omitting AIX. It works but not reliably.
if (common.isLinux || common.isOSX || common.isWindows) {
const dir = common.tmpDir + '/watch';
const dir = `${common.tmpDir}/watch`;

@refackrefackJul 16, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is Ok.
But it would be much better to use path.join instead, see #14272 (comment)

@vsemozhetbyt

vsemozhetbyt commented Jul 16, 2017

Copy link
Copy Markdown
Contributor

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

@Trott

Trott commented Jul 17, 2017

Copy link
Copy Markdown
Member

CI is green. I'm +0 on using path.join() instead. I'm fine with it the way it is, and using path.join() might be a marginal improvement.

Look for lots of "converting template literals to path.join" changes at the next Code + Learn. :-P

@blade254353074

blade254353074 commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

@Trott Can you change the code? or only @Helianthus21 can do it?

@tniessen

tniessen commented Jul 17, 2017

Copy link
Copy Markdown
Member

@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 path.join would eliminate all of @Helianthus21's changes, so I would prefer not to alter his / her code.

@Helianthus21

Helianthus21 commented Jul 18, 2017

Copy link
Copy Markdown
ContributorAuthor

Hey guys, I have already used path.join() instead the template string literals.

refack
refack approved these changes Jul 18, 2017
View reviewed changes

@refackrefack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💯
Thank you very much!

tniessen
tniessen approved these changes Jul 18, 2017
View reviewed changes

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Helianthus21!

jasnell pushed a commit that referenced this pull request Jul 19, 2017
@Helianthus21@jasnell
test: replace string concat in test-fs-watchfile.js
6a587ad
PR-URL: #14287
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

jasnell commented Jul 19, 2017

Copy link
Copy Markdown
Member

Landed with a few nits fixed in 6a587ad.
Thank you!

@jasnelljasnell closed this Jul 19, 2017
addaleax pushed a commit that referenced this pull request Jul 22, 2017
@Helianthus21@addaleax
test: replace string concat in test-fs-watchfile.js
58afe52
PR-URL: #14287
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
@Helianthus21@Fishrock123
test: replace string concat in test-fs-watchfile.js
3bc7d2a
PR-URL: #14287
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleaxaddaleax mentioned this pull request Jul 24, 2017
Closed
@addaleaxaddaleax mentioned this pull request Aug 2, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@gireeshpunathilgireeshpunathilgireeshpunathil approved these changes
+2 more reviewers
@refackrefackrefack approved these changes
@gibfahngibfahngibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tniessentniessen

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

15 participants

@Helianthus21@joyeecheung@vsemozhetbyt@Trott@blade254353074@tniessen@jasnell@refack@benjamingr@targos@cjihrig@gireeshpunathil@gibfahn@MylesBorins@nodejs-github-bot