Object default values display JSON under help menu

[carsonreinke]

Using an any object as the default value displays a JSON version of that object. This might not be very user friendly.

Here is an example using IPv4 from ip-num package:

(default: {"bitSize":32,"maximumBitSize":"4294967295","type":1,"octets":[{"value":1},{"value":1},{"value":1},{"value":1}],"separator":".","value":"16843009"})

Consider changing JSON.stringify at https://github.com/tj/commander.js/blob/master/index.js#L1413 to a scheme utilizing .toString instead. The impact could be pretty severe though.

[shadowspawn]

Interesting idea.

Would work nicely when there is a custom .toString, like with ip-num. The fallback mode for .toString on uncustomised objects is not very user friendly either though. i.e. [object Object]

On a somewhat related note, there was an issue opened about whether a custom coercion function should be called on the default value rather than the default value being in final form.

(A work-around is to handle the default in your own code when the option is undefined.)

[carsonreinke]

@shadowspawn I'm assuming you are referring to #1036.

I feel the supplied default should be previously coerced, since it may hard to represent all desired default values directly from the command line.

I'm actually not sure toString is even the right method for this, since the value is actually a command line friendly representation of the data. Whatever that would be called.

Either way, I don't see a real simple solution. My work around was this:

function parseIP(value: string): IPv4 | string {
    return IPv4.fromDecimalDottedString(value);
}

program.option('--dns <ip>', 'Additional DNS server to check', parseIP, '1.1.1.1');

...

if(typeof(program.dns) === 'string') {
    program.dns = parseIP(program.dns);
}

[shadowspawn]

I finally found the comment where someone expected the default value to be processed by the coercion function : #400 (comment)

[carsonreinke]

@shadowspawn Thank you. Looking into this more, I thought that coercing the default value would be the best, but it appears it is more complicated. The coercion function also takes a previous argument, coercing the default value, there is no previous value.

Ultimately from the test cases, the default value should be previously coerced. The easiest would be to just add some more complex "stringify" functionality.

Maybe something like this:

switch(typeof(option.defaultValue)) {
  case 'undefined': return '';
  case 'string':
  case 'boolean':
  case 'number':
    return JSON.stringify(option.defaultValue);
  default /*object, etc*/: return option.defaultValue.toString();
}

Thoughts?

[shadowspawn]

(The previous argument to the coercion function is optional. But changing the help is much less breaking than changing the default processing, so the more likely change!)

An additional check could be to only use toString is it is not the default toString (which would return [object Object]).

[shadowspawn]

(Reproducing a comment from discussion in #1309.)

I see yargs handles this by having a default and a defaultDescription:

default: value, set a default value for the option, see default()
defaultDescription: string, use this description for the default value in help content, see default()

with a nice example of the function flavour:

.default('timeout', 60000, '(one-minute)')

This is a simple direct approach to the help problem and separates the value from the help representation. There is not anywhere currently to add properties like this, but I have been wondering about how to make options more extensible for the less common things: #518 #1106

[shadowspawn]

Experimenting with Option approach in #1331:

program
   .addOption(new Option('-t, --timeout <delay>').default(60, 'one minute'))

[carsonreinke]

@shadowspawn that sounds wonderful, I will take a look at #1331.

[shadowspawn]

Included in v7.0.0 pre-release #1386

[shadowspawn]

Included in Commander 7.0.0.