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

Optional type and Custom option processing do not work together #984

Closed
r4j4h opened this issue Jun 27, 2019 · 6 comments
Closed

Optional type and Custom option processing do not work together #984

r4j4h opened this issue Jun 27, 2019 · 6 comments

Comments

@r4j4h
Copy link

r4j4h commented Jun 27, 2019

This section of the docs indicates

You can specify an option which functions as a flag but may also take a value (declared using square brackets).

and then Custom option processing docs indicates

You may specify a function to do custom processing of option values. The callback function receives two parameters, the user specified value and the previous value for the option. It returns the new value for the option.

This allows you to coerce the option value to the desired type, or accumulate values, or do entirely custom processing.

You can optionally specify the default/starting value for the option after the function.

Currently custom options behave differently based on if an optional argument is provided. The examples only show no argument and required argument (<example>) but no optional argument ([example]).

I believe the code is being confused and treating optional arguments as required arguments and this is making it skip calling the custom option processor for any entries without an argument. This is error-prone and to me, unexpected and hopefully unintended.

I have tried with both 2.22.0 and 3.0.0 and the issue behaves the same.

I want to effectively make an incremental verbosity flag that also lets users specify direct levels, or add them up, e.g. -d -d -d === -d3 === -d -d2 but with this behavior this seems to be impossible, or else I am misunderstanding something.

Here is how to reproduce, using the script from the documentation:

const program = require('commander');

program
  .option('-c, --cheese [type]', 'Add cheese with optional type');

First modify it to use custom options, like so, we just want to debug output to observe what is happening, so we hard return false because it does not matter.

const program = require('commander');

function customProcessor(value, previous) {
    console.log('value', value, 'previous', previous);
    return false;
}

program
    .option('-c, --cheese [type]', 'Add cheese with optional type', customProcessor, false);

program.parse(process.argv);

Let's first demonstrate application behavior when [type] is present in the option definition.

run with script -c -c swiss --cheese american -c -c gruyere
I see

value swiss previous true
value american previous false
value gruyere previous true

now try with [type] removed:

const program = require('commander');

function customProcessor(value, previous) {
    console.log('value', value, 'previous', previous);
    return false;
}

program
    .option('-c, --cheese', 'Add cheese with optional type', customProcessor, false);

program.parse(process.argv);

run with script -c -c swiss --cheese american -c -c gruyere
I see

value undefined previous false
value undefined previous false
value undefined previous false
value undefined previous false
value undefined previous false

The important part to me is that with [type] the processor gets called three times, but without it it gets called each time. I can safely handle the presence of the value and cumulating them in my processor, but only if I get called each time. If I do get called each time, I must also be given the arguments, otherwise I cannot even read the given optional argument.


Extra case

While I do not need the required value case, I thought it would be good to try it as well, and it actually behaves way different, so I am including it here:

Change the script as so:

const program = require('commander');

function customProcessor(value, previous) {
    console.log('value', value, 'previous', previous);
    return false;
}

program
    .option('-c, --cheese <type>', 'Add cheese with optional type', customProcessor, false);

program.parse(process.argv);

As before, when run with
run with script -c -c swiss --cheese american -c -c gruyere
I now see an alarming

value -c previous false
value american previous false
value -c previous false

I tried changing the defaults from false to sensible strings but that didn't change anything but the logged value from false to the string.

@shadowspawn
Copy link
Collaborator

By happenstance, I was looking at this code last night and had some idea of the cause. After some digging...

When the hybrid option with optional value is used as a flag, an event is emitted with a null argument:

this.emit('option:' + option.name(), arg);

The event handler that calls the custom option processing skips the call if the argument is null, so your routine does not get called:

if (val !== null && fn) {

While it makes some sense that a "coercion" function is not called if there is no value, the custom option processing is being used for more than simple coercion now. This might be an omission when the code evolved. (History might help make sense of whether it was accidental, to do.)

@shadowspawn
Copy link
Collaborator

Turned out to be a good theory, the test for null dates back to the conversion from the simpler coercion function to the more flexible custom option processing:

edafae1

@shadowspawn
Copy link
Collaborator

An alternative approach could be to use an event listener, which does get called for both types of call.

const program = require("commander");

var level = 0;

program
  .option("-v,--verbose [increment]", "increase verbosity")
  .on("option:verbose", (value) => {
    if (value === null)
      level = level + 1;
    else
      level = level + parseInt(value);
  });

program.parse(process.argv);

console.log(`Verbosity level: ${level}`);
$ node index.js 
Verbosity level: 0
$ node index.js -v -v -v
Verbosity level: 3
$ node index.js -v 2
Verbosity level: 2
$ node index.js -v -v 2 -v
Verbosity level: 4 

@r4j4h
Copy link
Author

r4j4h commented Jul 5, 2019

Great thinking with the event listener! I like that approach a lot, and hadn't even noticed it in the documentation 😊

@shadowspawn
Copy link
Collaborator

I think we could improve the custom option processing for options with optional values, but as it would be a breaking change, and we found a work-around, closing this issue.

(Future readers) Feel free to open a new issue if it comes up again, with new information and renewed interest.

Thank you for your contributions.

@shadowspawn
Copy link
Collaborator

I have a PR open to allow custom option processing to work with optionals with .preset(), to specify a preset argument which does get passed to the custom option processing.

const { program, Option } = require("commander");

program
  .addOption(new Option("-v,--verbose [increment]", "increase verbosity")
    .default(0)
    .preset("1")
    .argParser((arg, previousLevel) => {
      return previousLevel + parseInt(arg);
    })
  );

program.parse(process.argv);

console.log(`Verbosity level: ${program.opts().verbose}`);
% node preset.js -v -v -v 4
Verbosity level: 6

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

No branches or pull requests

2 participants