Skip to content

Fix to fail if required arguments don't exist.#941

Closed
yossan wants to merge 4 commits intotj:masterfrom
yossan:fix_missing_arguments
Closed

Fix to fail if required arguments don't exist.#941
yossan wants to merge 4 commits intotj:masterfrom
yossan:fix_missing_arguments

Conversation

@yossan
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

See also: #896

@shadowspawn shadowspawn removed the request for review from segfaultmedaddy June 22, 2019 02:02
@yossan
Copy link
Copy Markdown
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.

2 participants