FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[JsConf Code & Learn]test: replace string concatenation with template literals by nathansmile · Pull Request #14322 · nodejs/node · GitHub

/ node Public

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

Closed
nathansmile wants to merge 0 commit into
nodejs:masterfrom
nathansmile:master
ConversationCommits (0)ChecksFiles changed
Closed

[JsConf Code & Learn]test: replace string concatenation with template literals#14322
nathansmile wants to merge 0 commit into
nodejs:masterfrom
nathansmile:master

Conversation

nathansmile commented Jul 17, 2017

Copy link
Copy Markdown
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 added the test Issues and PRs related to the tests. label Jul 17, 2017
tniessen reviewed Jul 17, 2017
View reviewed changes
Comment thread test/parallel/test-icu-data-dir.js Outdated

tniessen Jul 17, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This change is not necessary.

nathansmile Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ok, thks for suggestion.

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 requested changes Jul 17, 2017
View reviewed changes

Trott 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread test/parallel/test-icu-data-dir.js Outdated

Trott Jul 17, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This might be slightly better as:

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

tniessen Jul 17, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

nathansmile Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thks, I submit the new PR in #14342

tniessen commented Jul 18, 2017

Copy link
Copy Markdown
Member

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

nathansmile commented Jul 18, 2017

Copy link
Copy Markdown
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 commented Jul 18, 2017

Copy link
Copy Markdown
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 commented Jul 18, 2017

Copy link
Copy Markdown
Member

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 commented Jul 19, 2017

Copy link
Copy Markdown
Contributor Author

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tniessen tniessen left review comments
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

Back | FazBrowse Home | New Git URL