Bugfix for #10 - "options containing hyphens unrecognized"

[da70]

Bugfix for options containing hyphens unrecognized · Issue #10 · dthree/vorpal, including test.

This is the simplest possible fix, though perhaps not the most regression-proof one. I will add some notes in a comment in the issue explaining other possible options.

[dthree]

This is excellent - thanks!

As it's a very touch part of the code, I'm going to do some more testing before pulling, will get to this as soon as I can.

[dthree]

You've also got some breaking tests, looks like linting.

[da70]

Hi,

Just submitted a PR for this. Yesterday I was trying to add a --dry-run option and noticed it wasn't passing through. I debugged and found the cause and came up with various solutions, then found this open ticket.

The cause:

In util.buildCommandArgs, this line:

      var long = String(o.long || '').replace(/--no-/g, '').replace(/-/g, '');

...normalizes o.long to "dryrun", but when this option was first defined in util.parseArgs, it was normalized using minimist to "dry-run", so the match in util.buildCommandArgs here fails:

      exist = exist === undefined && parsedArgs[long] !== undefined ? parsedArgs[long] : exist;

In the PR I just submitted I opted for a simple, direct fix, changing .replace(/-/g, '') to .replace(/^-*/g, '') so that only leading dashes are replaced. (I just now noticed I forgot to remove the /g, which is now superfluous).

The more bulletproof solution would be to use parsedArgs in both places, perhaps something like this:

function normalizeArg(str) {
      var obj = minimist([str]);
      var normalizedArg;

      for (var key in obj) {
        if (obj.hasOwnProperty(key)) {
          if (key !== '_') {
            normalizedArg = key;
            break;
          }
        }
      }

      return normalizedArg;
    };

...

      var long = normalizeArg(String(o.long || '').replace(/--no-/g, ''));

I did test this and it worked, but you can see it's a bit awkward due to the fact that parseArgs, like minimist, takes an array and returns an object with an added _ element. Also, it sounds like the option parsing is going to be refactored at some point. If it's desired, though, I can submit a separate PR that uses this approach.

I just now noticed that Travis CI builds failed with the same error I've been getting on my machine on gulp build. To run the tests and get them to pass on my machine I had to edit dist/util.js as well as lib/util.js (but only checked in the latter, obviously). This bugfix should work once gulp build is working again.

Hope this helps. It would be great to have the ability to use hyphenated options because there are a lot of standard ones out there already.

Thanks. Am really liking working with Vorpal so far.

[da70]

I think that's a spurious error on Travis - the build appears to be failing there. I couldn't get master to gulp build on my machine, either. This was before I even made any changes. Not sure what's causing it.

[da70]

On my machine (with dist/util.js edited because can't build):

$:vorpal/> git log -1
commit fc90cb072a0c0acc379a1f360e762452575331b5
Merge: d492362 6f1ff60
Author: David <da70@nyu.edu>
Date:   Fri Feb 19 18:12:19 2016 -0500

    Merge branch '10_options-containing-hyphens-unrecognized'
$:vorpal/> git diff
diff --git a/dist/util.js b/dist/util.js
index bd04910..cf449dc 100755
--- a/dist/util.js
+++ b/dist/util.js
@@ -244,7 +244,7 @@ var util = {
     for (var m = 0; m < cmd.options.length; ++m) {
       var o = cmd.options[m];
       var short = String(o.short || '').replace(/-/g, '');
-      var long = String(o.long || '').replace(/--no-/g, '').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 && o.required !== 0) {
$:vorpal/> mocha 2>/dev/null | egrep 'hyphenated|passing'
    ✓ should parse hyphenated options
  60 passing (116ms)

[dthree]

Thanks. I'll check out the failing soon and will take a further look.

Really appreciate the help.

It would be great to have the ability to use hyphenated options because there are a lot of standard ones out there already.

Yeah, totally! Just ran into this a week ago:

https://github.com/vorpaljs/cash/blob/master/src/commands/mv.js#L96

[da70]

Glad I can be of some help. Vorpal is a very cool project.

[dthree]

Sorry for the delay. Doing a number on arg parsing now, and will inclue this as part of it!

[da70]

No worries. Looking forward to it, thanks!

[dthree]

Thanks boss.

[dthree]

Available in v1.10.6.

[da70]

It works like charm, thanks!

[dthree]

Awesome!