Repository: mattallty/Caporal.js (ts)

Issue:
Function validators only work if they return promises
**Describe the bug**
Due to the change in [6c67c95](https://github.com/mattallty/Caporal.js/commit/6c67c95) validator functions that return a value, rather than a promise, cause the option to be set to undefined as the value is not returned by the `_validateWithFunction()` function.

**To Reproduce**
1. Create an option with a function validator. For example the simple example below:
```js
function list(value: any): string[] {
    if (typeof value !== 'string') {
        throw new Error(`Invalid value: '${value}'`);
    }
    return value.split(',');
}
```
2. Run the command with the option set, the value comes out as undefined.

**Expected behavior**
My understanding is that the previous change should not have required validator functions to use promises as the examples in the README still return values directly. As such, validator functions that return values rather than promises should also function as they did previously. 

**Environment informations (please complete the following information):**
 - OS: Mac
 - OS version: Mojave
 - Shell: Bash
 - Caporal version: 1.2.0


Pull request:
fix(validator): Fix function validators for options
This pull request fixes an issue with options using custom validator functions where values in the
option object were set to undefined. This pull also includes new tests which check that options are
set correctly to prevent a regression in the future.

Closes #149

Patch:
diff --git a/lib/command.js b/lib/command.js
index 56d2f50..9b4e084 100644
--- a/lib/command.js
+++ b/lib/command.js
@@ -399,7 +399,7 @@ class Command extends GetterSetter {
 
       const finishProcessingOptions = (_opts) => {
         // add long notation if exists
-        opts = this._addLongNotationToOptions(opts);
+        opts = this._addLongNotationToOptions(_opts);
         // camelcase options
         opts = this._camelCaseOptions(opts);
 
diff --git a/lib/validator.js b/lib/validator.js
index 33fcf5e..aaba47f 100644
--- a/lib/validator.js
+++ b/lib/validator.js
@@ -152,6 +152,8 @@ class Validator {
       const result = this._validator(value);
       if (isPromise(result)) {
         return result.catch(e => throwAsInvalid(value, e));
+      } else {
+        return result
       }
     } catch(e) {
       throwAsInvalid(value, e);


Test patch:
diff --git a/tests/option-validation.js b/tests/option-validation.js
index 250563d..1f3b1b5 100644
--- a/tests/option-validation.js
+++ b/tests/option-validation.js
@@ -123,11 +123,17 @@ describe('Passing --option valid-value', () => {
   ['regex', 'function', 'STRING', 'INT', 'BOOL', 'BOOL(implicit)', 'FLOAT', 'LIST(int)', 'LIST(bool)', 'LIST(float)'].forEach(function(checkType) {
     it(`should succeed for ${checkType} check`, () => {
       if (checkType === 'regex') {
+        program.action(function (args, options) {
+          should(options.time).eql('234');
+        });
         program.option('-t, --time <time-in-secs>', 'Time in seconds', /^\d+$/);
         program.parse(makeArgv(['-t', '234']));
 
-      } else if(checkType === 'function') {
-        program.option('-t, --time <time-in-secs>', 'Time in seconds, superior to zero', function(val) {
+      } else if (checkType === 'function') {
+        program.action(function (args, options) {
+          should(options.time).eql(2);
+        });
+        program.option('-t, --time <time-in-secs>', 'Time in seconds, superior to zero', function (val) {
           const o = parseInt(val);
           if (isNaN(o) || o <= 0) {
             throw new Error("FOOOO")
@@ -136,35 +142,59 @@ describe('Passing --option valid-value', () => {
         });
         program.parse(makeArgv(['-t', '2']));
 
-      } else if(checkType === 'STRING') {
+      } else if (checkType === 'STRING') {
+        program.action(function (args, options) {
+          should(options.file).eql('foo')
+        });
         program.option('-f, --file <file>', 'File', program.STRING);
         program.parse(makeArgv(['-f', 'foo']));
 
-      } else if(checkType === 'INT') {
+      } else if (checkType === 'INT') {
+        program.action(function (args, options) {
+          should(options.time).eql(282);
+        });
         program.option('-t, --time <time-in-secs>', 'Time in seconds', program.INT);
         program.parse(makeArgv(['-t', '282']));
 
-      } else if(checkType === 'BOOL') {
+      } else if (checkType === 'BOOL') {
+        program.action(function (args, options) {
+          should(options.happy).eql(true);
+        });
         program.option('--happy <value>', 'Am I happy ?', program.BOOLEAN);
         program.parse(makeArgv(['--happy', 'yes']));
 
-      } else if(checkType === 'BOOL(implicit)') {
+      } else if (checkType === 'BOOL(implicit)') {
+        program.action(function (args, options) {
+          should(options.happy).eql(true);
+        });
         program.option('--happy', 'Am I happy ?', program.BOOLEAN);
         program.parse(makeArgv(['--happy']));
 
-      } else if(checkType === 'FLOAT') {
+      } else if (checkType === 'FLOAT') {
+        program.action(function (args, options) {
+          should(options.time).eql(2.8);
+        });
         program.option('-t, --time <time-in-secs>', 'Time in seconds', program.FLOAT);
         program.parse(makeArgv(['-t', '2.8']));
 
-      } else if(checkType === 'LIST(int)') {
+      } else if (checkType === 'LIST(int)') {
+        program.action(function (args, options) {
+          should(options.list).eql([1, 8]);
+        });
         program.option('-l, --list <list>', 'My list', program.LIST | program.INT);
         program.parse(makeArgv(['--list', '1,8']));
 
-      } else if(checkType === 'LIST(bool)') {
+      } else if (checkType === 'LIST(bool)') {
+        program.action(function (args, options) {
+          should(options.list).eql([true, false, true, false, true, false]);
+        });
         program.option('-l, --list <list>', 'My list', program.LIST | program.BOOL);
         program.parse(makeArgv(['--list', 'true,0,yes,no,1,false']));
 
-      } else if(checkType === 'LIST(float)') {
+      } else if (checkType === 'LIST(float)') {
+        program.action(function (args, options) {
+          should(options.list).eql([1.0, 0]);
+        });
         program.option('-l, --list <list>', 'My list', program.LIST | program.FLOAT);
         program.parse(makeArgv(['--list', '1.0,0']));
       }
@@ -176,9 +206,10 @@ describe('Passing --option valid-value', () => {
   });
 
   it(`should succeed for promise check`, done => {
-    program.action(function() {});
+    var time = 0;
+    program.action(function (args, options) { time = options.time });
 
-    program.option('-t, --time <time-in-secs>', 'Time in seconds, superior to zero', function(val) {
+    program.option('-t, --time <time-in-secs>', 'Time in seconds, superior to zero', function (val) {
       return new Promise((resolve, reject) => {
         setTimeout(() => {
           const o = parseInt(val);
@@ -190,11 +221,15 @@ describe('Passing --option valid-value', () => {
       })
     });
     // then.catch.then to ensure the assertion is made whether the promise resolves or not (simulates `finally` behavior for node 6 & 8)
-    program.parse(makeArgv(['-t', '2'])).then(() => {}).catch(() => {}).then(() => {
+    program.parse(makeArgv(['-t', '2'])).then(() => { }).catch(() => { }).then(() => {
       const count = error.callCount;
-      should(count).be.eql(0);
-
-      done();
+      try {
+        should(count).be.eql(0);
+        should(time).be.eql(2);
+        done();
+      } catch (e) {
+        done(e);
+      }
     })
   });
 });