Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 26, 2017
Merged

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

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.

None yet

3 participants