Support custom error messages for option argument coercion failures

[shadowspawn]

Pull Request

Problem

It can be tricky to do nice error messages for custom option processing failures since the option name is not available.

Issues: #1207 #518 (comment)

Solution

We already added a custom exception to process the error message for the new .choices() routine. This takes that a little further to make the exception usable by custom option processing routines and not just Option methods.

For example:

const commander = require('commander');
const program = commander.program;

program
  .option('-p, --port <port>', 'port number', (arg) => {
    const portNumber = parseInt(arg);
    if (isNaN(portNumber)) {
      throw new InvalidOptionArgumentError('Must be a number.');
    }
    return portNumber;
  });

program.parse();

options % node throw.js -p       
error: option '-p, --port <port>' argument missing
options % node throw.js -p 123   
options % node throw.js -p Madrid
error: option '-p, --port <port>' argument 'Madrid' is invalid. Must be a number.

Release Note

• Added: throwInvalidOptionArgumentError from custom option processing for detailed error message.

To Do

• README
• tests
• example
• TypeScript

[shadowspawn]

FYI: @lydell @vonagam

[lydell]

Looks great! Exactly what I need. Though maybe a little more low-level than I anticipated. [Edit: refers to older version of code, with this comment leading to changes.]

Two thoughts:

• I think this should be mentioned in the readme docs.

• In my project, I would define the following helper function:

function invalidArgument(message) {
  return new commander.CommanderError(1, 'commander.optionArgumentRejected', message);
}

// Later, somewhere:
throw invalidArgument('Must be a number.')

Do we want a shorter way to do this out of the box? I’m thinking you can’t customize the exit code for other errors, such as “missing option”, right? So maybe we don’t need that ability here. Is there a use case for using any other code than 'commander.optionArgumentRejected'?

[shadowspawn]

Though maybe a little more low-level than I anticipated.

Fair comment, and good feedback thanks. I was trying a quick repurpose of the existing internals. 😊

[shadowspawn]

Added somewhat higher level wrapper:

  .option('-p, --port <port>', 'port number', (arg) => {
    const portNumber = parseInt(arg);
    if (isNaN(portNumber)) {
      throw new commander.InvalidOptionArgumentError('Must be a number.');
    }
    return portNumber;
  })

[lydell]

Looks good!

[shadowspawn]

Finished missing pieces, ready for review.

[abetomo]

I appreciate it.