Skip to content

modified test-repl-persistent-history to use common.mustCall - #12703

Closed
cool88 wants to merge 1 commit into
nodejs:masterfrom
cool88:master
Closed

modified test-repl-persistent-history to use common.mustCall#12703
cool88 wants to merge 1 commit into
nodejs:masterfrom
cool88:master

Conversation

@cool88

@cool88cool88 commented Apr 27, 2017

Copy link
Copy Markdown
Contributor
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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Apr 27, 2017
cjihrig
cjihrig previously requested changes Apr 27, 2017

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.

Could you leave this as it was please.

@cool88cool88May 2, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'll do so. I removed the function because this way runTest was accounting for an additional call. I thought it'd be an extra burden on the reader if he sees common.mustCall(runTest, numtests + 1) later on. Sorry, this was my first contribution on the git via NodeTodo, I am figuring out how to undo these changes as recommended.

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.

This comment can be dropped.

@mscdexmscdex added the repl Issues and PRs related to the REPL subsystem. label Apr 27, 2017
@Fishrock123Fishrock123 self-assigned this Apr 27, 2017
Fishrock123
Fishrock123 previously requested changes Apr 27, 2017

@Fishrock123Fishrock123 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.

I seem to recall not having done this for some specific reason but now I'm not sure what it was if it was anything...

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.

Could you please name the wrapped version something else and update the call points respectively?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Can you please look at it now? Thank you. I hope it was Ok to --amend this change?

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.

I hope it was Ok to --amend this change?

Yeah that's fine

@cool88
cool88force-pushed the master branch 2 times, most recently from 9549df3 to ef63593CompareMay 2, 2017 18:17
@Trott

Trott commented May 5, 2017

Copy link
Copy Markdown
Member

This LGTM if CI is green. Ping @cjihrig@Fishrock123

@Trott

Trott commented May 5, 2017

Copy link
Copy Markdown
Member

@TrottTrott 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.

LGTM if CI is green

@addaleax
addaleax dismissed stale reviews from cjihrig and Fishrock123May 7, 2017 21:07

addressed

@addaleax

Copy link
Copy Markdown
Member

Landed in 6058c43, thanks for the PR! :)

@addaleaxaddaleax closed this May 7, 2017
addaleax pushed a commit that referenced this pull request May 7, 2017
PR-URL: #12703
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#12703
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnelljasnell mentioned this pull request May 11, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #12703
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #12703
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

replIssues and PRs related to the REPL subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@cool88@Trott@addaleax@jasnell@Fishrock123@cjihrig@gibfahn@mscdex@nodejs-github-bot