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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 0 commits into from
Closed

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

wants to merge 0 commits 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)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 17, 2017
tniessen
tniessen reviewed Jul 17, 2017
View reviewed changes
test/parallel/test-icu-data-dir.js Outdated
@@ -7,8 +7,8 @@ const assert = require('assert');
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');
`could not initialize ICU (check NODE_ICU_DATA or ` +
Copy link
Member

Choose a reason for hiding this comment

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

This change is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thks for suggestion.

@tniessen tniessen added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 17, 2017
Trott
Trott requested changes Jul 17, 2017
View reviewed changes
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.

Thanks for submitting this PR! The change in line 10 is not needed. The change in line 11 looks good to me, although I left a comment suggesting something that might be a bit better still. Can you update the PR at least to remove the changes in line 10?

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

Choose a reason for hiding this comment

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

This might be slightly better as:

  `--icu-data-dir parameters)${os.EOL}`;

Copy link
Member

Choose a reason for hiding this comment

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

That's actually my fault, we had to fast-track a fix for #13940 in #13987.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thks, I submit the new PR in #14342

@tniessen
Copy link
Member

@nathansmile Did you intentionally close this PR? It seems like you force-pushed the upstream master branch instead of your changes.

@nathansmile
Copy link
Contributor Author

When I'm updating the code, I don't want to add a new commit, or there will be 2 commits for this issue (I think it's not necessary). So I hard reset and force push the branch to my origin branch. After that I found this PR closed by me. What a surprise! Then I submit another PR.
Also, I was finding the help doc, but can't find the detail help for update changes.
If this bring trouble, I'm sorry for that!
Thank you for suggestion and helping~

@vsemozhetbyt
Copy link
Contributor

@nathansmile If you want to just update the last commit in a branch, you can try:

git checkout your-branch
(do the change)
git commit --amend
(change the commit message if needed)
git push --force-with-lease

The last commit will be updated in the PR automatically.

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

So I hard reset and force push the branch to my origin branch. After that I found this PR closed by me.

FWIW I think once you push a new commit onto the branch you can reopen the PR. The easier fix is not to push between resetting and making the new commit.

@nathansmile
Copy link
Contributor Author

@vsemozhetbyt Thanks a lot~ That is what I need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen left review comments

@Trott Trott Trott requested changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nathansmile @tniessen @vsemozhetbyt @gibfahn @Trott @nodejs-github-bot