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

Inherit arguments to default subcommand #614

Closed
wants to merge 5 commits into from

Conversation

Justinidlerz
Copy link

@Justinidlerz Justinidlerz commented Apr 4, 2017

You can use the option on the subcommand.Like so:

pm.js

var program = require('commander');
program
  .command('default', 'default command', { noHelp: true, isDefault: true });

program
  .command('server')
  .option('-p --port <port>', 'port', /^\d+$/, 3000)
   .action(port => {
     // start a server with the option port
   })
  .parse(process.argv);

pm-default.js

#!/usr/bin/env node
var program = require('commander');

program
    .option('-t --test <number>', 'Test inherit subcommand option', /^\d+$/, 500)
    .parse(process.argv);

console.log('default');
console.log(program.test);

RUN

./pm.js -t 800 // echo default 800

@Justinidlerz Justinidlerz changed the title inherit unknow args to 'defaultExecutable' inherit args to subcommand Apr 4, 2017
@Justinidlerz Justinidlerz changed the title inherit args to subcommand Inherit args to subcommand Apr 4, 2017
@Justinidlerz Justinidlerz changed the title Inherit args to subcommand Inherit arguments to subcommand Apr 4, 2017
@Justinidlerz Justinidlerz changed the title Inherit arguments to subcommand Inherit arguments to default subcommand Apr 4, 2017
@shadowspawn
Copy link
Collaborator

I was bit concerned whether this was a good idea, but I see it requested in #461, and I checked yargs and it allows options for default command, so will be considered further.

@shadowspawn shadowspawn self-requested a review May 13, 2019 09:31
@shadowspawn
Copy link
Collaborator

I have a concern that unrecognised commands will get passed to default hander as arguments. For example with commands server and default:

# 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.

@shadowspawn
Copy link
Collaborator

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.

@shadowspawn shadowspawn removed their request for review June 22, 2019 05:04
@shadowspawn
Copy link
Collaborator

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.

@shadowspawn shadowspawn self-requested a review August 10, 2019 06:15
Copy link
Collaborator

@shadowspawn shadowspawn left a 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!

  1. I do not wish to change the main executable example examples/pm. I consider a default a less common use.
  2. stale changes to package.json

Given it is a one line change + tests, might be easier to redo a new PR against develop branch?

@shadowspawn
Copy link
Collaborator

@Justinidlerz
Are you interested in reworking this, or should I take it over?

@shadowspawn
Copy link
Collaborator

shadowspawn commented Aug 10, 2019

Stripped down pm.js

var program = require('commander');

program
  .command('default', 'default command', { noHelp: true, isDefault: true })
  .parse(process.argv);

pm-default.js

#!/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

@Justinidlerz
Copy link
Author

@shadowspawn Sorry, it's a loooooong time PR, I forgot the contents. Let me see again it to recall.

@shadowspawn shadowspawn added this to the v4.0.0 milestone Sep 14, 2019
@shadowspawn shadowspawn added the semver: major Releasing requires a major version bump, not backwards compatible label Sep 15, 2019
@shadowspawn
Copy link
Collaborator

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.

@shadowspawn
Copy link
Collaborator

Closing in favour of #1047 which finishes this work.

@shadowspawn
Copy link
Collaborator

#1047 has been merged to develop and will be released in v4.0.0.

Thank you for your contributions.

@shadowspawn
Copy link
Collaborator

v4.0.0 has been released.

Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Releasing requires a major version bump, not backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants