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

[JsConf Code & Learn]test: replace string concatenation with template literals #14342

Closed
wants to merge 1 commit into from

Conversation

nathansmile
Copy link
Contributor

replace string concatenation in test/parallel/test-icu-data-dir.js with template literals

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@@ -8,7 +8,7 @@ const { spawnSync } = require('child_process');

const expected =
'could not initialize ICU (check NODE_ICU_DATA or ' +
'--icu-data-dir parameters)' + (common.isWindows ? '\r\n' : '\n');
`--icu-data-dir parameters)${common.isWindows ? '\r\n' : '\n'}`;
Copy link
Member

@gibfahn gibfahn Jul 18, 2017

Choose a reason for hiding this comment

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

While you're here, could you replace ${common.isWindows ? '\r\n' : '\n'} with ${os.EOL} (you'll have to const os = require('os') after line 2 as well)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually since expected is later used with str.includes(expected) you could remove the EOL completely, and fit the string in a single line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gibfahn sure, it will come soon~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@refack Sorry, I can't get your meanings. Do you mean the code afterconst expected ... to be one line?

Copy link
Member

@gibfahn gibfahn Jul 20, 2017

Choose a reason for hiding this comment

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

@refack means this:

-    '--icu-data-dir parameters)' + (common.isWindows ? '\r\n' : '\n');
+    '--icu-data-dir parameters)'

but it was fine as it is (and arguably slightly better).

Copy link
Contributor

Choose a reason for hiding this comment

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

@nathansmile sorry I missed your comment. But I landed it since it's good. 😄

@refack
Copy link
Contributor

refack commented Jul 18, 2017

@nathansmile thank you for the contribution. Hope you follow up on this and make it even better 👍

@refack refack self-assigned this Jul 18, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 19, 2017
@joyeecheung
Copy link
Member

@nathansmile I think @refack meant since expected is tested with str.includes, we do not actually expect an exact match here so the test should still pass without an EOL at the end of expected. Although I think it still does not fit into one single line after dropping the EOL..

@joyeecheung
Copy link
Member

BTW personally I think we should keep the EOL in expected because we would actually want to make sure there is a line break here...otherwise the output might look a bit awkward.

Copy link
Member

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

@joyeecheung
Copy link
Member

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

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.

refack pushed a commit to refack/node that referenced this pull request Jul 19, 2017
PR-URL: nodejs#14342
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor

refack commented Jul 19, 2017

Landed in 01eddd9

@refack refack closed this Jul 19, 2017
addaleax pushed a commit that referenced this pull request Jul 22, 2017
PR-URL: #14342
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
PR-URL: #14342
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jul 24, 2017
@addaleax addaleax mentioned this pull request Aug 2, 2017
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants