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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(command): builder function no longer needs to return the yargs instance #549

Merged
merged 3 commits into from Aug 7, 2016

Conversation

Projects
None yet
3 participants
@nexdrew
Member

nexdrew commented Jul 11, 2016

Fixes #522.

I'm not quite sure about the intentions of what should be allowed to be returned by the builder function. Returning the yargs instance or nothing should now work, but I wonder if we ever intended to allow it to return something like yargs.argv? If so, I think we will need a new issue for that.

Otherwise, this PR seems to work fine, but it could possibly result in unexpected behavior in user CLIs if they were previously not returning the yargs instance, either by mistake or not. For that reason, this change may be better suited for 5.x.

Note that I skipped the populates argv with placeholder keys when passed into command handler unit test because this change breaks it and it no longer seems to be valid (options must now be marked global: true in order to apply to the argv of a command handler, be it a placeholder or populated value). We should either remove it (if everyone agrees the test is no longer valid), or fix it by using global: true (which is tested elsewhere).

UPDATE: With commit 739a4f4, a builder function can also call yargs.argv, with or without returning. I think this is a good idea b/c I've seen a few folks try to do this and it makes the API much less rigid. 馃槑

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 11, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 800ce3c on command-builder-no-return into eb1e185 on master.

coveralls commented Jul 11, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 800ce3c on command-builder-no-return into eb1e185 on master.

Show outdated Hide outdated test/command.js
it('does not require builder function to return', function () {
var argv = yargs('yo')
.command('yo [someone]', 'Send someone a yo', function (yargs) {

This comment has been minimized.

@bcoe

bcoe Jul 14, 2016

Member

how is yo app doing, have they raised a 1 billion $$$ series B yet?

@bcoe

bcoe Jul 14, 2016

Member

how is yo app doing, have they raised a 1 billion $$$ series B yet?

@bcoe bcoe added the 5.x label Jul 14, 2016

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Jul 14, 2016

Member

Is the solution this simple, I could have sworn I thought of an edge-case that would bite us and forced the 5.x release; I'll look at this pull when I'm fresh on the weekend -- I think we're really close to 5.x either way.

Member

bcoe commented Jul 14, 2016

Is the solution this simple, I could have sworn I thought of an edge-case that would bite us and forced the 5.x release; I'll look at this pull when I'm fresh on the weekend -- I think we're really close to 5.x either way.

@maxrimue maxrimue referenced this pull request Aug 7, 2016

Closed

Version 5.0.0 #519

10 of 10 tasks complete

@bcoe bcoe merged commit eaa2873 into master Aug 7, 2016

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@bcoe bcoe deleted the command-builder-no-return branch Aug 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment