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 to fail if required arguments don't exist. #941

Closed
wants to merge 4 commits into from

Conversation

yossan
Copy link

@yossan yossan commented Mar 29, 2019

The missingArgument method isn't called now.

// <file> should be required but it works now.
program
  .argument('<file>')
  .parse(['node', 'test'])

These commits call the missingArgument method.

error: missing required argument `file`

@shadowspawn
Copy link
Collaborator

I have not worked through the code yet. A couple of questions @yossan (but you don't have to answer, it won't block my review, just make it easier):

  • does this work correctly with multiple arguments too? .argument('<file> <env>')
  • does this improve same situation with subcommands?

@yossan
Copy link
Author

yossan commented Apr 1, 2019

@shadowspawn

does this work correctly with multiple arguments too? .argument(' ')

// error: missing required argument `env'
program
    .arguments('<file> <env>')
    .parse(['node', 'test', 'setup'])
program
    .arguments('<file> <env>)
    .parse(['node', 'test', 'setup', 'path'])

does this improve same situation with subcommands?

This doesn't work.

program
    .command('cmd <file>')
    .parse(['node', 'test', 'cmd'])

I apologize. This commit may have caused subcommands not to work well.
I will fix it as soon as possible.

@shadowspawn
Copy link
Collaborator

See also: #896

@shadowspawn shadowspawn removed the request for review from roman-vanesyan June 22, 2019 02:02
@yossan
Copy link
Author

yossan commented Jul 15, 2019

@shadowspawn
I issued #995.

I think a fundamental review is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants