-
-
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
Inherit arguments to default subcommand #614
Conversation
I was bit concerned whether this was a good idea, but I see it requested in #461, and I checked |
I have a concern that unrecognised commands will get passed to default hander as arguments. For example with commands # oops, server typo calls default command with argument servers
./pm.js servers -p 90 Basically the fallback error handling shifts from the outer program to the default command. Heavier weight change than might first appear. |
I still have mixed feelings about this one. I am leaving it open but not considering it for v3, so no action likely for a while. |
I was concerned about what I thought was changing behaviour to call default subcommand when an option was specified, but was looking at that code recently and saw that this is exactly what happens if an unrecognised argument is included on the command line such as when the default command takes an argument. Happier that this change is consistent with existing behaviour and will be taking another look after v3. |
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.
Finally convinced myself this is appropriate!
- I do not wish to change the main executable example
examples/pm
. I consider a default a less common use. - stale changes to
package.json
Given it is a one line change + tests, might be easier to redo a new PR against develop branch?
@Justinidlerz |
Stripped down var program = require('commander');
program
.command('default', 'default command', { noHelp: true, isDefault: true })
.parse(process.argv);
#!/usr/bin/env node
var program = require('commander');
const util = require('util')
console.log('Called default subcommand with')
console.log(process.argv.slice(2));
program
.arguments('<file>')
.option('-a, --all')
.action((file) => {
console.log(`File is ${file}`);
})
.parse(process.argv);
console.log(`option all is ${program.all}`); Behaviour before fix: $ node pm.js
Called default subcommand with
[]
option all is undefined
$ node pm.js x
Called default subcommand with
[ 'x', '[object Object]' ]
File is x
option all is undefined
$ node pm.js --all
error: unknown option '--all'
$ node pm.js x --all
Called default subcommand with
[ 'x', '[object Object]', '--all' ]
File is x
option all is true After fix: $ node pm.js
Called default subcommand with
[]
option all is undefined
$ node pm.js x
Called default subcommand with
[ 'x', '[object Object]' ]
File is x
option all is undefined
$ node pm.js --all
Called default subcommand with
[ '--all' ]
option all is true
$ node pm.js x --all
Called default subcommand with
[ 'x', '[object Object]', '--all' ]
File is x
option all is true |
@shadowspawn Sorry, it's a loooooong time PR, I forgot the contents. Let me see again it to recall. |
I'm going to start work on this for 4.0.0 in a new PR. I'll add you (@Justinidlerz) as co-author on the commit. |
Closing in favour of #1047 which finishes this work. |
#1047 has been merged to develop and will be released in v4.0.0. Thank you for your contributions. |
v4.0.0 has been released. Thank you for your contributions. |
You can use the option on the subcommand.Like so:
pm.js
pm-default.js
RUN
./pm.js -t 800 // echo default 800