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

[jhao]

…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)

[Trott]

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]

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?

now splits at sentence break

[Trott]

LGTM if CI is green.

[vsemozhetbyt]

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

[vsemozhetbyt]

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]

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]

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	[email protected]:gibfahn/node.git (fetch)
# origin	[email protected]: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]

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

[gibfahn]

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

[jhao]

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

[vsemozhetbyt]

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

[Trott]

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

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

[jhao]

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

[gibfahn]

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

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

[Trott]

CI failures appear unrelated. This can land.

[jasnell]

Landed in b923b9d