Fix help default values display

[carsonreinke]

Pull Request

Problem

Fixes #1283.

Default values for options that are not either string or boolean display as JSON encoded string.

An example would be using IPv4 from ip-num package produces the following as the default value:

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

This value is not helpful or user friendly.

Solution

Allow for default values that are objects to utilize a custom toString method to display for the help. The default Object.toString method will not be used. All other types of default values will continue to use JSON.stringify.

NOTE: This did require a signature change of TypeScript typings to communicate that allows for defaultValues to be any instead of just string | boolean.

ChangeLog

N/A

[shadowspawn]

I like the look of the key code change, nice and small. I haven't tried running it yet, but optimistic enough from reading the code that I jumped straight to suggestions on tuning the PR!

[carsonreinke]

@shadowspawn I made some updates, let me know if those work!

[shadowspawn]

Turns out JSON.stringify is very robust! When I tried putting in some challenging default values, the new code crashed displaying the help. e.g.

.option('--null <value>', 'null', null)
.option('--cleanObject <value>', 'Object.create(null)', Object.create(null))

I did a bit of reading and experimentation and suggest:

    if (typeof value === 'object' && value !== null && typeof value.toString === 'function' && value.toString !== Object.prototype.toString) {

[shadowspawn]

For interest, I ran through various standard data types looking for ones that would change display.

Array
stringify: ["a","b","c"]
toString: a,b,c

URL
stringify: "http://example.com/"
toString: http://example.com/

Date
stringify: "2020-07-24T10:33:12.776Z"
toString: Fri Jul 24 2020 22:33:12 GMT+1200 (New Zealand Standard Time)

[shadowspawn]

I am slightly worried that this will not be an improvement in some cases, but I like the idea in principle of calling toString rather than JSON.stringify as closer to the intent — displaying as text.

Needs tightening up for the test whether appropriate to call .toString, otherwise looking tidy now.

[carsonreinke]

@shadowspawn Oh yes, these are some good cases, I will update.

Though, starting to think toString is trying to force it and maybe something like toCommandLine or toTerminal would be better. This is how JSON.stringify works with allowing the toJSON method. Anyway you look at it could be a breaking change, especially since you don't know if someone intentionally wanted to use toString or did not.

[carsonreinke]

@shadowspawn updated! Let me know your thoughts.

[shadowspawn]

Hmm, I had noticed the stringify use of toJSON too and am having second thoughts about toString given we both have some concerns! I will play with some alternative roll-your-own solution without core code changes as wondering if it is simple enough to avoid making a core change with mixed benefits.

[carsonreinke]

@shadowspawn I just feel toString really is for human readable content, not necessarily machine parseable. Let me see what I can come up with and please let me know if you have any ideas.

[shadowspawn]

Two work-around approaches for reference: a slightly hacky one taking advantage of JSON.stringify looking for toJSON, and a simple one handling it manually.

const { program } = require('commander');
const ipv4 = require('ip-num').IPv4;

const defaultValue = new ipv4("1.1.1.1");
const customValue = new ipv4("2.2.2.2"); 
customValue.toJSON = () => { return customValue.toString() }
const manualValue = '3.3.3.3';

program
  .option('--plain <value>', 'default Commander behaviour', defaultValue)
  .option('--custom <value>', 'custom toJSON', customValue)
  .option('--manual <value>', `manual (default: "${manualValue}")`);
program.parse();

const opts = program.opts();
opts.manual = opts.manual || new ipv4(manualValue);
console.log(opts);

[carsonreinke]

@shadowspawn I know what you are saying, the problem is re-purposing toJSON to something it is not.

My feeling now is that focus should be instead on fixing problems presented in #400. That way, the default value argument is actually forced to be a string. The user supplies the command line friendly value and it gets processed by the user supplied parse function.

Would you be open to closing this and going in the direction of #400 instead?

[shadowspawn]

Changing the behaviour so that the custom processing (coercion) function is called on the default/starting value is likely to work better with displaying the starting value for rich coercions. I am intrigued with how it might work. I don't think it is as natural for simpler coercions though, say needing to specify "3.4" instead of 3.4. And a big warning in advance, I suspect it will be a very breaking change and not compelling enough to break existing users.

I noticed 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 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]

Thanks @carsonreinke for good discussion. I initially thought toString might work out well, but your comment summed up what I felt too after getting into the details:

starting to think toString is trying to force it