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

Add support for command-specific yargs-parser option specs #464

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jannis
Copy link
Contributor

@Jannis Jannis commented Mar 6, 2019

This allows any command to specify how its options are parsed, based on the opts argument for yargs-parser:

module.exports = {
  description: '...',
  options: {
    boolean: ['with-x'],
  },
  run: async toolbox => ...
}

Without this, invokations like

mycli cmd --with-x arg1 arg2

would result in arg1 being consumed as the value for --with-x, and toolbox.parameters.first being set to arg2. To me this is undesirable, because it really complicates handling input arguments in any non-trivial command.

The new options feature allows commands to control how their arguments are parsed, making it possible for

mycli cmd --with-x arg1 arg2

to parse to { withX: true, 'with-x': true } options, with toolbox.parameters.first being arg1 and toolbox.parameters.second being arg2.

The hard part of this change is detecting commands correctly without having parsed the options first. This PR is a draft because the way I implemented this is maybe not elegant and robust enough. Basically, it scans all arguments (including options) and builds two separate arrays, one for the command path, the other for all the unrelated arguments. When encountering a -foo or --foo argument, it pushes it to the unrelated arguments array. The next value is then treated as a command path segment only if [...prefix, nextValue] is a valid command path. It is treated as an option value when encountering situations like --option cmd cmd however.

@skellock @jamonholmgren Curious to hear what you think!

@jamonholmgren
Copy link
Member

I like the idea in principle! I would like to see if it causes problems with nontrivial code bases like AWS Amplify CLI, Solidarity, and the upcoming Ignite CLI.

@Jannis
Copy link
Contributor Author

Jannis commented Mar 7, 2019

Agreed. The tests are non-exhaustive and I've only tried this with https://github.com/graphprotocol/graph-cli. My feeling is that unless there is a global CLI spec built from all commands and their options, there will always be a level of ambiguity.


Since I didn't want to fork gluegun just for this, I've implemented a simple heuristic in an upcoming feature for graph-cli to deal with the original problem:

let { withX } = toolbox.parameters.options
let array = fixParameters(toolbox.parameters, { booleanOptions: { withX } })

where fixParameters simply loops over all boolean props and builds a new parameter array from toolbox.parameters.array. If one of the boolean options has a string value, that value is pushed to the front of the parameters array. It's an OK workaround for now.


Still, if a command like mycli cmd --debug arg1 arg2 doesn't have arg1 and arg2 as first and second, that can result in really weird behavior and a requires CLI developers to come up with similar workarounds. And boolean flags like --verbose, --debug are pretty common.

Potential solutions / improvements:

  1. Restrict options to be used at the end. Rather unusual, wouldn't recommend.

  2. Restrict options to be used after the commands (that way supporting command.options would be straight forward). Basically:

    mycli [cmd ...] [options] [arg ...]

    but also allowing options and args to be swapped. First, parse [cmd ...] to identify the command, then parse the arguments based on the yargs-parser opts spec. Could break existing scripts that put options before commands. This would be my preferred solution.

  3. Build a global CLI arguments spec from all commands and their options to avoid ambiguity altogether. To avoid ambiguity, which options take values and which don't needs to be known upfront. Hard.

  4. Allow users to specify at least command.booleanOptions. Then, apply a similar logic to my fixParameters to the toolbox.parameters just before the command is executed. Could work, but 2. (supporting all yargs-parser opts) is more powerful and flexible.

@jamonholmgren
Copy link
Member

After thinking this over, I agree that doing it the way you explain in your original PR sounds the best. I'd love if you could bring this to a point where you feel it's testable, and then I'll go test it with Ignite and Solidarity. @Jannis thanks for your work!

@danawoodman
Copy link
Contributor

Is there a way to get something like this supported where we can define option types and have them auto-documented?

@jamonholmgren
Copy link
Member

@danawoodman Left a response on the other PR: #427 (comment)

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

Successfully merging this pull request may close these issues.

3 participants