Adds disallowUnknownOptions() method to command

[jeff-1amstudios]

Will display help if user enters an option that is not in the options array. Current default behavior is to silently ignore unknown options. If you have a command

.command('cmd')
.option('--a')
.disallowUnknownOptions()
.action(...)

Now if the user enters cmd --a --b, the output is Unknown option: b. Showing Help: .... The action is not invoked

[dthree]

Thanks. In a regular Linux terminal, the default for passing an additoinal option is:

cat: invalid option '--z'
Try 'cat --help' for more information.

In developing Vorpal, a design goal is to stick as close as possible to POSIX-compliance and familiarity of commands.

In other words, could you just make your edit the default? I don't want to add too many more things to the API.

I wouldn't call this a breaking change, as no one should be passing invalid commands to Vorpal for no reason :)

---

Otherwise, your code itself looks great! :+1:

[jeff-1amstudios]

Sure, I agree that it should be the default, but wanted to avoid breaking existing behavior. Do we need to keep a .allowUnknownOptions() in command.js? Seems useless now the default is false (and unknown options don't get passed to your action method anyway)

[dthree]

I was thinking about it... I almost don't want to - It just seems like it would contribute to making Vorpal overwhelming.

You see these frameworks where a brand new person visiting sees these relatively obscure methods to handle rare edge cases, and they get blown off. So let's leave it out and just adopt your (better) functionality.

---

Can you see any possible case where it would break anything? I can't, but I could be wrong.

[jeff-1amstudios]

:+1: fully agree. Can't see any downside (unless my calculation of unknown options is wrong ;) )

[dthree]

:+1: So just make those few changes and send me a ping, and I'll pull and publish right away!

[jeff-1amstudios]

Is it documented anywhere the correct ordering of long/short options? If I have .option('-a', 'blah'), the underlying Option will be created as { flags: '-a', long: '-a' } which seems incorrect, and also breaks the detection of invalid options because we have a long value with a singe dash prefix.

I noticed this because the 'vorpal execution command execution should execute a long command with arguments' test fails because of this reason.

So the question is - is the test invalid? Or is the option short/long calculation wrong?

[dthree]

I think the wiki docs go over short, then long:

.option('-a, --aardvarks', 'summons a horde of aardvarks');

Unless I'm mistaking your question?

---

My option / arg parsing code isn't canon - It's probably the sloppiest part of Vorpal IMHO, as I copy pasta'd a lot of it from Commander so probably didn't take as much time as I should have to understand it. So if you see something wrong, it's probably wrong. I'd like to do an overhaul on it at some point.

[jeff-1amstudios]

Digging a bit more - is it valid to only pass '-a' (without --aardvark)? That is the case which leads to an Option with { long: '-a' }

[dthree]

It should be! The behavior on the user end is that whether -a or --aardvarks are passed, it always comes out on the user end as options: {aardvarks: true}.

i.e. it will give the developer the longest option, so they don't have to check which one was passed.

[jeff-1amstudios]

yep, ok. Then we probably need to make https://github.com/dthree/vorpal/blob/3b54cfcfa856b1969bdee175d39fdbe731d88ef9/lib/option.js#L24-L27 a bit smarter about setting the correct long/short properties

[dthree]

Yeah probably. Haha look - I just pulled this out of Commander:

/**
 * Initialize a new `Option` with the given `flags` and `description`.
 *
 * @param {String} flags
 * @param {String} description
 * @api public
 */

function Option(flags, description) {
  this.flags = flags;
  this.required = ~flags.indexOf('<');
  this.optional = ~flags.indexOf('[');
  this.bool = !~flags.indexOf('-no-');
  flags = flags.split(/[ ,|]+/);
  if (flags.length > 1 && !/^[[<]/.test(flags[1])) this.short = flags.shift();
  this.long = flags.shift();
  this.description = description || '';
}

Literally copy paste.

[jeff-1amstudios]

Haha! The simplest fix would be to inspect the first 2 chars of each flag and assign it to the correct variable. Will do that now

[dthree]

K.

[jeff-1amstudios]

Done. If you don't see any problems with it I will squash the commits so you can merge

[dthree]

I'm ready to merge. You?

[jeff-1amstudios]

Yep! I was going to squash the commits but actually they fix 2 different things, so will leave them separated.

[dthree]

Okay thanks, you're awesome. :+1:

---

Published in v1.5.6.

[jeff-1amstudios]

great, thanks for quick merge+publish :)