Option function and default parameter is not executed

[eliperelman]

In source diving the arguments passed to command.option, it appears that it accepts a 3rd and 4th parameter for defining a function and default value for the option, but it appears that these aren't actually being executed. In this test case, I was unable to get a default value or have the function execute. Am I doing something wrong, or is this just not supported yet?

#! /usr/bin/env node

let Vorpal = require('vorpal');
let cli = new Vorpal();

cli
  .command('test')
  .description('A test')
  .option('--url <url>', 'Some url', function(val) { cli.log('VALUE!', value); }, 'http://some.url')
  .action(function(args, done) {
    this.log('URL:', args.options.url);
    done();
  });

cli.parse(process.argv);

$ ./cli test
URL: undefined

$ ./cli test --url other.url
URL: other.url

[dthree]

Haha - interesting find.

No, those aren't currently supported (what you find in the wiki is essentially what is supported), but it's funny it's there.

I copied a lot of the command logic originally out of TJ's Commander.js, and it looks like this may have been supported by him, and I never noticed.

Do you have need of this?

[eliperelman]

I do. I currently use the deprecated nomnom package which allows you to specify an option's default value if not specified, and also a function, which you can use for validation or transforming logic. I can write a wrapper around nomnom to handle this, but that means having to duplicate the vorpal plugin logic, which I'd rather not do. It would be nice to switch from nomnom to Vorpal.

[dthree]

Okay cool. Could you do me a favor and detail the exact expected logic of the function? i.e. at what point in the action's lifecycle is it executed, etc.?

[eliperelman]

According to commander, it appears it is done during the parseOptions method:

https://github.com/tj/commander.js/blob/master/index.js#L690

which I assume maps to command._parseExpectedArgs util.buildCommandArgs, but that's just a cursory guess.

[dthree]

K I'll see if I can get to this soon.

[eliperelman]

Experimented with a patch, which seemed to work well with my test case:

let Vorpal = require('vorpal');
let cli = new Vorpal();

let transform = function(val, defaultValue) {
  if (val && !val.startsWith('http')) {
    return 'http://' + val;
  }

  return val;
};

cli
  .command('test')
  .description('A test')
  .option('--url <url>', 'Some url', transform, 'http://some.url')
  .action(function(args, done) {
    this.log('URL:', args.options.url);
    done();
  });

cli.parse(process.argv);

./cli test
URL: http://some.url

./cli test --url github.com
URL: http://github.com

./cli test --url https://github.com
URL: https://github.com

diff --git a/lib/util.js b/lib/util.js
index e37309e..40693d7 100755
--- a/lib/util.js
+++ b/lib/util.js
@@ -191,15 +191,20 @@ var util = {
     // and throws help.
     for (var m = 0; m < cmd.options.length; ++m) {
       var o = cmd.options[m];
+      var name = o.name();
       var short = String(o.short || '').replace(/-/g, '');
       var long = String(o.long || '').replace(/--no-/g, '').replace(/-/g, '');
-      var exist = (parsedArgs[short] !== undefined) ? parsedArgs[short] : undefined;
-      exist = (exist === undefined && parsedArgs[long] !== undefined) ? parsedArgs[long] : exist;
-      if (exist === true && o.required !== 0) {
+      var value = parsedArgs[long || short] || cmd[name];
+
+      cmd.emit(name, value);
+      value = cmd[name];
+
+      var exist = value !== undefined;
+      if (exist === false && o.required !== 0) {
         return '\n  Missing required option. Showing Help:';
       }
-      if (exist !== undefined) {
-        args.options[long || short] = exist;
+      if (exist) {
+        args.options[long || short] = value;
       }
     }

Thoughts?

[eliperelman]

What's interesting about this approach is that the same function passed to .option could be used for validation as well. Maybe it could be sniffed to see if value instanceof Error, then return the error message instead of "missing required option". Just an idea.

[jescalan]

+1 for being able to specify defaults!

[eliperelman]

@dthree any more thoughts on this? Just had this come up on another project that would be useful.