-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 variadic arguments #277
Conversation
I made sure to use existing convention for style/error handling/... |
Thanks! Could you also update the docs? |
Of course. |
Do you have anything specific in mind? Want an example in the README.md, update the annotated source, both? |
both, please! |
Done. I amended the commit to avoid you having multiple entries in your Git log. Also, the examples are as convoluted as the others but I tried to make sure to explain the important parts:
Please let me know if I can do more. |
@@ -178,7 +209,7 @@ Examples: | |||
- [more progress bars](https://github.com/substack/node-multimeter) | |||
- [examples](https://github.com/visionmedia/commander.js/tree/master/examples) | |||
|
|||
## License | |||
## License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh - nice catch here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't on purpose, thank Atom.io. I didn't even catch this when reviewing the diff. ;)
Docs look great as well - you may want to add a quick definition of if @thethomaseffect @zhiyelee have no objections, we can bring this in. |
Looks good to me. I'm of the impression this wasn't added before because you could accomplish the same thing with type coercion (e.g. 'command arg1,arg2,arg' and then a string split to get the array) but that's not very unix-y so I'm all in favor of adding this. LGTM 👍 |
|
||
if (argDetails.name.indexOf('...') === argDetails.name.length - 3) { | ||
argDetails.variadic = true; | ||
argDetails.name = argDetails.name.slice(0, argDetails.name.length - 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slice(0, -3)
is enough.
@SomeKittens @thethomaseffect I have reviewed the PR. After fixed, I am all for merging this. Big thanks to @whitlockjc |
Not sure if it's important in this context, but the "rest" function argument in ES6 uses a prepended ellipses instead of appended. In other words, ES6 would say "...arguments" to collect the remainder of arguments in a function while this PR uses "arguments..." to indicate similar for a command. Just thought I'd mention it in case anyone feels it warrants discussion. |
I have updated the PR with the review feedback, making sure to adhere to existing conventions. I hope I didn't overlook anything else but if I did, let me know and I'll get it sorted. |
@whitlockjc Thanks for the new commit, and how do you think about which @theganyo mentions? |
Unix tools puts the ellipsis after the variable name for variadic arguments and since this library is for building command line interfaces, adhering to the Unix approach makes most sense for me. |
Add support for variadic arguments
Sounds good. I have merged it. Thanks for your contribution again @whitlockjc . |
That's what open source is about. If I can help further, let me know. |
@zhiyelee An updated version soon would be very useful, just ran into needing that today. |
@Globegitter v2.5.0 had been released. |
@zhiyelee thank you that is very much appreciated. |
This is a quick attempt at fixing issue #53. It works by allowing the user to mark the last argument as variadic by appending
...
to the argument name. Here is an example:.command('mycommand <requiredArg> [variadicArg...]')
. If you attempt to mark an argument as variadic but it is not the last argument, you will get an error. Here is an example:.command('mycommand <variadicArg...> [nonVariadicArg]')
. (As I type out this message, I realize it could be possible to get fancy and allow commingling of variadic and regular arguments where possible but it would get pretty tricky and I think this is a good start.)Also, whenever the
action
is called, the arguments are proper such that the variadic argument is anArray
of arguments. So if I were to invoke my command like thisnode myapp mycommand arg0 arg1 arg2 arg3
, my action function signature could look like thisfunction (requiredArg, variadicArg)
andrequiredArg
would have a value of'arg0'
whilevariadicArg
would have a value of['arg1', 'arg2', 'arg3']
.program.args
is also proper such that its value would be this:['arg0', ['arg1', 'arg2', 'arg3'], [Object object]
where the last argument is the program itself.Fixes #53