Skip to content

test: replace string concatenation with template literals - #15924

Closed
jphblais wants to merge 3 commits into
nodejs:masterfrom
jphblais:master
Closed

test: replace string concatenation with template literals#15924
jphblais wants to merge 3 commits into
nodejs:masterfrom
jphblais:master

Conversation

@jphblais

@jphblaisjphblais commented Oct 6, 2017

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@TrottTrott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@mscdexmscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Oct 6, 2017

const moduleMap = new ModuleMap();

assert.throws(() => moduleMap.get({}), errorReg);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could DRY this file up a bit if you did something like:

[{},[],true,1,()=>{}].forEach((value)=>{assert.throws(()=>moduleMap.get(value),errorReg);});

@refack

refack commented Oct 7, 2017

Copy link
Copy Markdown
Contributor

Welcome @jphblais and thank you for the contribution 🥇

Quick CI:
https://ci.nodejs.org/job/node-test-commit-linuxone/9075/ ✔️
https://ci.nodejs.org/job/node-test-linter/12291/ ✔️

@refackrefack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DRY would be nice.


assert.throws(() => moduleMap.has(1), errorReg);

assert.throws(() => moduleMap.has(() => {}), errorReg);

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.

Non-blocking nit: I would love it if your assertions were not separated by empty lines as long as they do not use more than a single line themselves.

@BridgeAR

Copy link
Copy Markdown
Member

@jphblais would you also be so kind and remove the merge commit by rebasing and force pushing the branch?

@refackrefack self-assigned this Oct 9, 2017
@lance

lance commented Oct 9, 2017

Copy link
Copy Markdown
Member

@BridgeARBridgeAR assigned BridgeAR and unassigned refackOct 18, 2017
BridgeAR pushed a commit that referenced this pull request Oct 19, 2017
PR-URL: #15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Oct 19, 2017
PR-URL: #15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR

Copy link
Copy Markdown
Member

Landed in 3b90bd0 and f05a2d8.

Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 !

MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15924
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.testIssues and PRs related to the tests.whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@jphblais@refack@BridgeAR@lance@cjihrig@tniessen@vsemozhetbyt@mscdex@MylesBorins@Trott@nodejs-github-bot