Skip to content

fix: running parse() multiple failed when help() enabled#790

Merged
bcoe merged 1 commit into7.xfrom
fix-776
Feb 26, 2017
Merged

fix: running parse() multiple failed when help() enabled#790
bcoe merged 1 commit into7.xfrom
fix-776

Conversation

@bcoe
Copy link
Member

@bcoe bcoe commented Feb 21, 2017

there was a bug that caused an exception to be raised when parse() was called multiple times on the same yargs instance if help() is enabled.

fixes #776

@bcoe bcoe requested a review from JaKXz February 21, 2017 04:38
Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

LGTM, my questions are more out of curiousity than blockers.

counter++
})
.help()
y.parse(['foo'], function () {})
Copy link
Member

@JaKXz JaKXz Feb 21, 2017

Choose a reason for hiding this comment

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

related but tangential question: should the fn passed to parse be optional? i.e. if no fn is passed in, use a noop?

Choose a reason for hiding this comment

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

It is optional, but specifying something prevents yargs from outputting to stdin (my possibly limited understanding of how it works).

Copy link
Member Author

Choose a reason for hiding this comment

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

@JaKXz, @scriptdaemon is correct; if you provide a function, we assume that you want to hook into output -- rather than the output of parse being printed to stdout.

This is useful for chat-bots, which is @scriptdaemon use-case.

it('allows command handler to be invoked repeatedly when help is enabled', function (done) {
var counter = 0
var y = yargs([])
.command('foo', 'the foo command', {}, function (argv) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know the test code well enough, but, my preference would be a method that is spied on and then asserted that it's been called twice, possibly outside the second parse call.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@JaKXz I like this idea ... but I also have yet to pull sinon into the codebase. I think pulling in a mocking framework is beyond the scope of this pull, but I do like the idea. a couple questions come to mind:

  • is there something lighter-weight than sinon that's popular?
  • I think we should probably pull in a stub/mock framework in the context of a larger test cleanup.

Copy link
Member

Choose a reason for hiding this comment

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

@bcoe I'm a big fan of https://github.com/testdouble/testdouble.js/ - and yeah I think it'd do nicely in a larger cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JaKXz awesome, perhaps we could eyeball the test-suite sometime soon, and see how many places we're using ad-hock spies -- if we're doing it a bunch of times, I say let's try testdouble.js out.

@bcoe bcoe merged commit 21b1e35 into 7.x Feb 26, 2017
@bcoe bcoe deleted the fix-776 branch February 26, 2017 06:21
bcoe added a commit that referenced this pull request Feb 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants