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

WIP: Rework action parameters for more stable interface #1405

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Nov 30, 2020

Pull Request

Trying out an idea to improve code patterns now and in the future. Long description to work through the reasoning although the key code change is only a few lines of code!

Problem

In Commander v6 the action handler may get passed based on settings and unexpected args:

  • expected args + command with options as properties
  • expected args + options
  • expected args + command without options as properties
  • any of above + array of extra args

This has not worked out as nicely as hoped for moving to storing options safely!

  1. When the options were always stored on the command and it was passed to action handler (as in older versions), the action handler gets a command parameter with everything on it. When the options are stored separately, we want it to be easy and convenient to access the options values, but still tidy to access the command.

Always always passing in command and calling cmd.opts() would only be one extra line of code, but would appear in most action handlers in most programs, and feels like paying for the increased safety.

  1. There are now multiple variations for passed options/command, so implementations will vary depending on the settings.

  2. There are now multiple variations for passed options/command, so hard to write examples that work whatever the settings. (This is a downside I notice as a maintainer!)

  3. With the extra args only sometimes present, the parameters are not consistent so trickier to write a generic action handler. e.g. Extra arguments : document and add configuration to fail if they exist #1268 (comment)

Solution

Always pass the action handler:

  • expected args + options + command

For backwards compatibility, the "options" might actually also be the command! The command still has options on it so the pattern still works.

This means code can access the options without an extra line of code in the common case, and easily access the command if needed without making configuration changes (just declare the parameter). The parameters are not affected by extra args so easier to write generic code if needed.

const program = require('commander');

program
  .command('sub [param]')
  .option('-d, --debug')
  .action((param, options, cmd) => {
    if (options.debug) {
      console.log(cmd.opts());
      console.log(cmd.args)
    }
  console.log(`Called sub with "${param}"`);
  });

program.parse();
$ node index.js sub --debug a b c
{ debug: true }
[ 'a', 'b', 'c' ]
Called sub with "a"

The one outlier might be if settings are .storeOptionsAsProperties(false) but .passCommandToAction(true) which some people will have adopted in Commander v6. For backwards compatibility will probably pass command + command to avoid breaking this, but will change the documentation so this is less likely in new code.

Related: CommandStrict to get all the recommended strict settings with latest best practices. #1404

ChangeLog

@shadowspawn
Copy link
Collaborator Author

On hold here, but being included in #1409

@shadowspawn shadowspawn closed this Dec 3, 2020
@shadowspawn shadowspawn deleted the feature/rework-action-parameters branch December 20, 2020 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant