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

[JSConf CN Code&Learn]test: replace string concatenation with template literals by jhao · Pull Request #14283 · nodejs/node · GitHub

/ node Public

[JSConf CN Code&Learn]test: replace string concatenation with template literals - #14283

Closed
jhao wants to merge 4 commits into
nodejs:masterfrom
jhao:17.Replace-string-concatenation-with-template-literals
ConversationCommits (4)ChecksFiles changed
Closed

[JSConf CN Code&Learn]test: replace string concatenation with template literals#14283
jhao wants to merge 4 commits into
nodejs:masterfrom
jhao:17.Replace-string-concatenation-with-template-literals

Conversation

jhao commented Jul 16, 2017 •
edited
Loading

Copy link
Copy Markdown

…tructor.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 16, 2017
joyeecheung added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 16, 2017
jhao changed the title replace string concatenation in test/parallel/test-child-process-cons… [JSConf CN Code&Learn]test: replace string concatenation with template literals Jul 16, 2017
Trott previously requested changes Jul 16, 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

Since we have to keep the split across two lines to stay under 80 chars per line, perhaps it makes sense to split at the sentence break?:

      message: 'The "options" argument must be of type object. ' +
               `Received type typeName(options)`

Trott commented Jul 16, 2017

Copy link
Copy Markdown
Member

I think I have a slight preference for keeping the one concatenation and indenting for readability:

      message: 'The "options" argument must be of type object. ' +
               `Received type typeName(options)`

But I'm good with this the way it is too. What do others think?

Trott dismissed their stale review July 16, 2017 08:15

now splits at sentence break

Trott approved these changes Jul 16, 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

LGTM if CI is green.

vsemozhetbyt commented Jul 16, 2017

Copy link
Copy Markdown
Contributor

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

vsemozhetbyt commented Jul 16, 2017

Copy link
Copy Markdown
Contributor

CIs for this PR fail (second try: https://ci.nodejs.org/job/node-test-pull-request/9171/) due to "merge conflict". Not sure how this can be fixed.

Trott commented Jul 17, 2017

Copy link
Copy Markdown
Member

CIs for this PR fail (second try: https://ci.nodejs.org/job/node-test-pull-request/9171/) due to "merge conflict". Not sure how this can be fixed.

@nodejs/build Any idea what's up? I recall seeing this before but I don't remember the cause and it was a very long time ago....

gibfahn commented Jul 17, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Not sure how this can be fixed.

Ideally by squashing and rebasing on master. @jhao if you're comfortable doing that let us know, otherwise a collaborator can do it.

Process is:

git remote -v # Should look like:
# ❯ git remote -v
# origin	git@github.com:gibfahn/node.git (fetch)
# origin	git@github.com:gibfahn/node.git (push)
# upstream	https://github.com/nodejs/node.git (fetch)
# upstream	https://github.com/nodejs/node.git (push)
git status # Make sure you're on `17.Replace-string-concatenation-with-template-literals`
git branch -u origin/17.Replace-string-concatenation-with-template-literals

git fetch --all
git rebase upstream/master
git push --force-with-lease

jhao commented Jul 17, 2017

Copy link
Copy Markdown
Author

@gibfahn Sure, let me do that, and push that again.

gibfahn commented Jul 17, 2017

Copy link
Copy Markdown
Member

@jhao if you could squash down to one commit that'd be good too.

jhao force-pushed the 17.Replace-string-concatenation-with-template-literals branch from 52048d2 to ada1399 Compare July 17, 2017 08:08

jhao commented Jul 17, 2017

Copy link
Copy Markdown
Author

@gibfahn I have squashed down to one commit about the "merge conflict"

vsemozhetbyt commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

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

Trott commented Jul 17, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@gibfahn The confusing part is that it seems like a phantom merge conflict. GitHub interface never reported a merge conflict. Guess I could/should check by rebasing next time and seeing if there's a merge conflict to see if it's Jenkins that is reporting incorrectly or GitHub's interface... (Or maybe I'm just misinterpreting one of the interfaces. ¯\(ツ)/¯)

jasnell reviewed Jul 17, 2017
View reviewed changes
Comment thread test/parallel/test-child-process-constructor.js Outdated
message: 'The "options" argument must be of type object. Received type ' +
typeName(options)
message: 'The "options" argument must be of type object. ' +
`Received type ${typeName(options)}`

jasnell 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

Please align the Received part with the The "options" part :-)

jhao Jul 19, 2017

Copy link
Copy Markdown
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

@jasnell I have fixed the code to align the Received part with the The "options" part

gibfahn commented Jul 18, 2017

Copy link
Copy Markdown
Member

@Trott I looked at the branch locally and there was a weird merge (looks like the branch got merged into itself).

I wouldn't entirely trust Github's UI, because it assumes you're going to merge the branch in rather than rebasing on top of master, so (I think) it only matters what head of the PR branch looks like. For a rebase (without squashing) every commit has to apply on top of master.

I'd rather just suggest git config --global pull.rebase true in CONTRIBUTING.md, but then you're encouraging people to git push --force-with-lease all the time, which could easily go wrong.

vsemozhetbyt commented Jul 19, 2017

Copy link
Copy Markdown
Contributor

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

Trott commented Jul 19, 2017

Copy link
Copy Markdown
Member

CI failures appear unrelated. This can land.

jasnell pushed a commit that referenced this pull request Jul 19, 2017
test: replace string concat in test-child-process-constructor
b923b9d 
replace string concatenation in test/parallel/test-child-process-constructor.js
with template literals

PR-URL: #14283
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Jul 19, 2017

Copy link
Copy Markdown
Member

Landed in b923b9d

jasnell closed this Jul 19, 2017
jasnell mentioned this pull request Oct 30, 2017
Merged
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

jasnell jasnell approved these changes
Trott Trott approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
gireeshpunathil gireeshpunathil approved these changes
+2 more reviewers
TimothyGu TimothyGu approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

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.

Back | FazBrowse Home | New Git URL