[JSConf CN Code&Learn]test: replace string concatenation with template literals#14283
[JSConf CN Code&Learn]test: replace string concatenation with template literals#14283jhao wants to merge 4 commits intonodejs:masterfrom
Conversation
Trott
left a comment
There was a problem hiding this comment.
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)`|
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? |
|
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.... |
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 |
|
@gibfahn Sure, let me do that, and push that again. |
|
@jhao if you could squash down to one commit that'd be good too. |
52048d2 to
ada1399
Compare
|
@gibfahn I have squashed down to one commit about the "merge conflict" |
|
@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. ¯\(ツ)/¯) |
| 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)}` |
There was a problem hiding this comment.
Please align the Received part with the The "options" part :-)
There was a problem hiding this comment.
@jasnell I have fixed the code to align the Received part with the The "options" part
|
@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 |
|
CI failures appear unrelated. This can land. |
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>
|
Landed in b923b9d |
…tructor.js with template literals
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)