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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Feb 26, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions test/command.js
View file
Open in desktop
Expand Up @@ -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) {
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.

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

@JaKXz JaKXz Feb 21, 2017 •
edited

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?

Copy link

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.

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')
Expand Down
2 changes: 1 addition & 1 deletion yargs.js
View file
Open in desktop
Expand Up @@ -957,7 +957,7 @@ function Yargs (processArgs, cwd, parentRequire) {
// consider any multi-char helpOpt alias as a valid help command
// unless all helpOpt aliases are single-char
// note that parsed.aliases is a normalized bidirectional map :)
var helpCmds = [helpOpt].concat(aliases[helpOpt])
var helpCmds = [helpOpt].concat(aliases[helpOpt] || [])
var multiCharHelpCmds = helpCmds.filter(function (k) {
return k.length > 1
})
Expand Down