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

Avoid help, version, validation, and the completion command while handling the completion option #178

Closed
wants to merge 3 commits into from

Conversation

tschaub
Copy link
Contributor

@tschaub tschaub commented Jun 17, 2015

While handling the completion option, parseArgs is called with a "partial" set of arguments. With strict parsing, these partial arguments may cause the task to exit with an error. This screws up completion. See #177 for an example script.

The changes here add a parseOnly argument to the parse method. While the completion values are being determined, yargs.parse(args, true) is called to avoid validation. This allows people to complete options whose partial values may not be valid options.

The test helper needed to be updated so that logging doesn't continue after process.exit() has been called. While processing the --get-yargs-completions option in the tests, the call to process.exit() doesn't stop execution, and errors were being logged about get-yargs-completions being an unknown argument. To make an assertion about there being no logged errors in the tests, the helper stops errors from being logged after process.exit() has been called.

Fixes #177.
Fixes #179.

@tschaub
Copy link
Contributor Author

tschaub commented Jun 17, 2015

It looks like option completion is also not working as expected with aliases. Given the following test.js script:

#!/usr/bin/env node
var argv = require('./index')
  .completion('completion')
  .help('help')
  .alias('help', 'h')
  .strict()
  .argv;

console.log('argv: %j', argv);

Setting up completion:

$ eval "$( ./test.js completion )"

And then typing ./test.js --h[TAB] results in this:

$ ./test.js --help, 

(note the trailing comma)

@tschaub
Copy link
Contributor Author

tschaub commented Jun 17, 2015

In 856db9c, the completion option is always handled before the help or version options. Previously, the order options were handled depended on the order of Object.getKeys(argv). So when an option starting with an h or a v was being completed, the help or version option handling might have come first (which is not what you want when you're passing the completion option).

It would be cleaner to separate the parse method into parse, validate, and process methods (where process would handle things like help and version). But I was hoping to minimize the changes in an effort to get in a fix for option completion.

@tschaub
Copy link
Contributor Author

tschaub commented Jun 18, 2015

In 2c64dd0, the completion option is handled before the completion command. Without this change, ./test.js completion [TAB] results in an infinite loop (see #179). While I know this is becoming harder to review, fixing this issue in this branch is a more straightforward change than fixing it in master.

@tschaub tschaub changed the title Avoid validation while handling completion Avoid help, version, validation, and the completion command while handling the completion option Jun 18, 2015
@bcoe
Copy link
Member

bcoe commented Jun 18, 2015

@tschaub whoops, I was on a long flight today and accidentally patched many of these same issues:

#181

I think I also solved: #179 on this branch, mind taking a look?

Sorry I stepped on your toes, would you be interested in tackling:

#180

Your work is very much appreciated 💯

@bcoe bcoe closed this Jun 18, 2015
@bcoe
Copy link
Member

bcoe commented Jun 18, 2015

We were 100% on the same page with regards to these fixes, I'm only opting for #181 since it has a patch for the infinite problem.

@tschaub
Copy link
Contributor Author

tschaub commented Jun 18, 2015

No problem. The changes in #181 look good. I'll test out the branch now.

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