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 choices to arguments #1458

Closed
Niryo opened this issue Jan 31, 2021 · 16 comments
Closed

add choices to arguments #1458

Niryo opened this issue Jan 31, 2021 · 16 comments
Assignees
Milestone

Comments

@Niryo
Copy link
Contributor

Niryo commented Jan 31, 2021

is it possible to have choices for arguments?

createCommand('pickColor').arguments('<color>'); //color choices: ['red', 'blue', 'green']
@shadowspawn
Copy link
Collaborator

No, choices for command-arguments are not supported.

(But an alternative is to make color an option instead of an argument.)

@Niryo
Copy link
Contributor Author

Niryo commented Jan 31, 2021

Is it intentionally not supported or is it in the backlog? If it's in the backlog we can discuss about the api and maybe i could create a PR

@shadowspawn
Copy link
Collaborator

At this stage it is a new suggestion.

I see .choices() as just one of the things that option-arguments support that command-arguments do not: default value, custom option processing (coerce), and choices.

Do any other CLI frameworks support any of these for command-arguments?

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 31, 2021

I had a quick look at argparse, and it configures command-arguments and option-arguments the same way:

Looks like also support in yargs via positional, which supports choices:

This is very different to how Commander currently works!

@shadowspawn
Copy link
Collaborator

Previous related issue: #971

@Niryo
Copy link
Contributor Author

Niryo commented Feb 1, 2021

My first intuition was that arguments should be treated the same as options in terms of validation and api, so it will be quite natural to have addArguments(new Argument().choices()), same as options, don't you think?

btw, just as a side note, you need to be aware that commander becomes (I would say even already) the top of his kind (at least among js tools), so soon you will not be able to check other solutions for understanding the standard, commander is on the point where it can define the standard:)

@shadowspawn
Copy link
Collaborator

shadowspawn commented Feb 1, 2021

I think following the option syntax may work pretty well, allowing description in a more consistent way, and defaults and coercion, and the extensible syntax to allow choices. Specifying the arguments with .arguments() or along with the command name in .command() still works fine as a shortcut for the simple cases. Reworking examples from the README:

// arguments and description
oldProgram
  .arguments('<username> [password]')
  .description('test command', {
    username: 'user to login',
    password: 'password for user, if required'
  });
newProgram
  .description('test command')
  .argument('<username>', 'user to login')
  .argument('[password]', 'password for user, if required');

// default
cmdUsingOpt
  .option('-c, --cheese <type>', 'add the specified type of cheese', 'blue');
cmdUsingArg
  .argument('[cheese-type]', 'add the specified type of cheese', 'blue')

// coerce
cmdUsingOpt
  .option('-f, --float <number>', 'float argument', parseFloat)
cmdUsingArg
  .argument('<number>', 'float argument', parseFloat)

// choices
cmdUsingOpt
  .addOption(new Option('-d, --drink <size>', 'drink size').choices(['small', 'medium', 'large']));
cmdUsingArg
  .addArgument(new CommandArgument('<drink-size>', 'drink size').choices(['small', 'medium', 'large']));

@Niryo
Copy link
Contributor Author

Niryo commented Feb 6, 2021

what is/are the correct test suite for testing the arguments? I want to duplicate all the relevant tests and make sure they are passing with the new syntax

@shadowspawn
Copy link
Collaborator

Be warned: I am not sure yet whether there is enough value to make what I suspect will be a lot of code changes. I have not seen many requests for the features this would add, so will be watching to see if this issue gets many 👍 .

You may want to open a PR with some code to show and discuss, but just warning you not to expect it will be accepted.

Is there a particular area you are interested in experimenting with first? You did ask about choices, but that does need quite a lot of new syntax with what we are considering, so you may have to start elsewhere.

There are base argument tests in:

  • command.action.test.js
  • command.variadic.test.js

But there are related tests in lots of other files, including the help related tests and the new .allowExcessArguments(). Command-arguments and options are the base building blocks of the CLI.

@Niryo
Copy link
Contributor Author

Niryo commented Feb 6, 2021

sure, I know you have the right to reject the pr:)
anyways, I think the value is not only the features themselves, the biggest value I see is making the API more concise. I am new to this lib, and it was quite un-intuitive for me to understand the difference between arguments and options, in term of API. after learning the Options api, I would expect to have the same things for arguments. also, I have a complex program with many sub-commands, and it's nice to have common arguments, just like I have common options (many commands use the same options and same arguments, like fileName argument). sadly, with the current implementation of arguments I can't have common arguments. Adding Choices and all the other sweet stuff is a nice bonus also:)

So I think I will start by introducing the new API without adding new capabilities like choices, and later I'll them.

@shadowspawn
Copy link
Collaborator

.argument() added in #1467 #1490 #1497 for Commander v8

@shadowspawn
Copy link
Collaborator

The support for defaults and custom parsing with .argument() has landed on release/8.x, which makes .choices() a simpler addition.

@shadowspawn
Copy link
Collaborator

Would you like to have first go at this @Niryo ?

@Niryo
Copy link
Contributor Author

Niryo commented May 5, 2021

@shadowspawn sure. i'll notice here when I am starting to work on it (until then if anyone see this and want to start working on it, you are welcome to do so)

@shadowspawn shadowspawn added this to the v8.0.0 milestone May 12, 2021
@shadowspawn
Copy link
Collaborator

I have made a start on this.

@shadowspawn shadowspawn self-assigned this May 21, 2021
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label May 26, 2021
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jun 25, 2021
@shadowspawn
Copy link
Collaborator

Released in Commander v8.

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

No branches or pull requests

2 participants