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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -747,6 +747,21 @@ describe('Command', function () { | |
counter.should.equal(2) | ||
}) | ||
|
||
// addresses: https://github.com/yargs/yargs/issues/776 | ||
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) { | ||
counter++ | ||
}) | ||
.help() | ||
y.parse(['foo'], function () {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
y.parse(['foo'], function () { | ||
counter.should.equal(2) | ||
return done() | ||
}) | ||
}) | ||
|
||
// addresses https://github.com/yargs/yargs/issues/522 | ||
it('does not require builder function to return', function () { | ||
var argv = yargs('yo') | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.