Error on unknown options and arguments if specified

[elliot-nelson]

SUMMARY

Allow program to specify that unknown arguments and options should result in an error.

DETAILS

My approach here is to use (assume?) that any slurped arguments that aren't claimed are unknown arguments. The check is made either (1) right before we call the run handler on the innermost command, or (2) when we're ready to return from parse, if no command was run.

I copied commander's interface here and used allowUnknown(true/false), although of course the default is true instead of commander's default of false. The boolean could easily be flipped and called preventUnknown or errorOnUnknown instead.

In my testing on my local copy, this approach seems to give the desired nesting behavior (you can specify .allowUnknown(false) in your top-level CLI as you define all your commands and get it everywhere, or you can specify it in the setup of just one command, if you don't want the behavior everywhere).

TESTS

I didn't add any unit tests yet, but am happy to if this approach looks reasonable and you'd want to merge it!

[nexdrew]

@elliot-nelson Wow, thanks for submitting this! I really appreciate you diving into the codebase to figure this out and for the detailed explanation - it really makes my job easier.

Offhand, this looks great, but I'd like to devote a little bit of time to a proper review, and I won't be able to do that until later this evening or perhaps tomorrow.

In the meantime, if you want to take a stab at some tests, that would be fantastic. I have every intention of getting this merged and released.

If you don't hear back with a review by Thursday morning (3/19/2020), please ping me here and I'll do my best to follow up. Since this stuff is fresh on my mind though, I'm gonna try to get you a review sooner rather than later.

Thanks again!

[elliot-nelson]

Well, in fact, I have done more testing and discovered a flaw -- I didn't notice at first that positional arguments don't end up "claimed" in the slurped args. So, any command that takes positional args incorrectly triggers the error.

That might be some kind of internal bug, but rather than hunt it down, I'll instead suggest my original idea -- which might be simpler. Before I came up with this approach, I had actually just made a tiny tweak to Context, in the populateArgv loop:

      tr.aliases.forEach(alias => {
        this.argv[alias] = tr.value
        this.knownArgv[alias] = tr.value
      })

This actually hews closer to the "if you can see it on usage, it's allowed" direction, and it correctly counts all positional arguments described in DSL strings (like .command("update <ip> <mac-address>")) correctly.

(If you go this direction, the only thing that has to change is my little slurp detection, which can become Object.keys(argv).filter(arg => !knownArgv.includes(arg)) etc.)

The bummer about the above is that you don't have the raw string, and for display purposes it would be preferable to print what the user actually typed (e.g. "--repiaar"). So there might be a little more tweaking to do to get it working right, to both detect the unknown args and get access to the raw values for display purposes.

[nexdrew]

Good point.

I like the approach of making this a part of the parseFromContext(context) workflow (instead of after parseFromContext completes) - because that gives us a chance to make the behavior specific to the command "level" (since parseFromContext executes once per command level) - either doing it during context.populateArgv() (as you mention) or sometime very near to it.

For the raw value, each object in context.slurped has a raw string property - can we use that?

[elliot-nelson]

I think so - I pushed up an extra commit to show a working example with tests.

• As the tests show, this approach does capture all unknown options in both standard and command form, and doesn't break anything. It doesn't "notice" misspelled commands (they'll drop to default command *) or extra positional arguments. If I remember correctly commander doesn't either, FWIW.

• getUnknownArguments() is now its own function available on context, and returns slurped objects. This might be a little bit unconventional (I don't think you expose slurped anywhere in the API today), but my goal is to make something that you could easily use in your own handler if you wanted a fancier version, e.g.:

.command({
    setup: sywac => {
        sywac
            .allowUnknown(true)  /* turn off built-in handling! */
    }),
    check: (argv, context) => {
        let unknown = context.getUnknownArguments();
        if (unknown.length > 0) {
            let likeliest = autocorrect(unknown[0].raw, Object.keys(context.knownArgv));
            context.cliMessage(`Unknown option ${unknown[0].raw}, did you mean ${likeliest}?`);
        }
    }
})

[nexdrew]

Cool!

There are sywac internals, but nothing is really "hidden" in terms of the API, so I'm good with exposing some view of slurped objects.

FYI I'm linting the codebase via standard, so you'll have to drop the semicolons. :)

[elliot-nelson]

I like the approach of making this a part of the parseFromContext(context) workflow (instead of after parseFromContext completes) - because that gives us a chance to make the behavior specific to the command "level" (since parseFromContext executes once per command level) - either doing it during context.populateArgv() (as you mention) or sometime very near to it.

Just a comment, if I remember correctly from my initial thrashing around 😆, it's hard to do this "inline" because at each level, options for the next level are considered unknown. (That is, options configured in the setup() of the lowest level subcommand, the one that actually runs, isn't assigned in populateArgv until you get there - which means 2-3 runs of parseFromContext would have considered it an unknown option.)

Running into that repeatedly is how I ended up with this strategy of waiting until we're about to run(), and then checking if there were unknown options.

[nexdrew]

That's true, it does make it more difficult, but the risk of doing it once at the top level after all parseFromContext executions complete is that:

1. We may lose the appropriate context, e.g. for suggesting a command-specific arg/flag
2. We may be checking it "too late" (i.e. if a command's run handler has already been called, which happens during the postParse phase of parseFromContext)

[elliot-nelson]

I've pushed up a new version of this PR that I think is even closer to what I'd like to see. It uses the same basic premise, which is that it detects the difference between known/unknown arguments while inside parseFromContext, and then it reports the errors in the furthest nested parse (for commands) or immediately once it returns to the root api (if there are no commands).

Improvements here:

• sywac is more like yargs than commander, so I've flipped the boolean and called it strict() instead of .allowUnknown(). By default strict mode is off, calling strict() turns it on.
• Strict mode now detects unknown options and unknown arguments, and calls them out with individual errors if detected. A positional argument that didn't match a known command will be reported as an unknown argument.

My testing so far shows behavior like what I'd expect from yargs, which is a bonus.

As implemented, command * is not compatible with strict(). I don't know if that's an issue or not - providing an arg that didn't match a command leaves it as unknown, and an unknown arg is not allowed, so it's not possible to drop into the "default" * command in strict mode. I feel like this might be OK for now, but can attempt to drill down further if not.

[elliot-nelson]

Oh and one extra note - I know there is already a concept of "strict" in the types. Although it's always tricky to overload a word I think they are separate enough not to be confused (strictness of a type's behavior vs strict defined at Api level).

[nexdrew]

@elliot-nelson Awesome, thanks!

Just want to let you know that I have not forgotten about this. I'm planning on reviewing this (and your docs PR) later this evening or tomorrow.

Thank you for your patience and helpful contributions! They are very much appreciated!

[nexdrew]

@elliot-nelson This is fantastic, nice work!

I noticed a slight "conflict of interest" 😁 with a .strict().showHelpByDefault() configuration, which is because .showHelpByDefault() applies a default "magic" command at each level, which you already pointed out the incompatibility between .strict() and default commands.

In general, I am ok with this incompatibility, since they seem like two different approaches to the same problem, but I think .showHelpByDefault() is common/important enough for command-based CLIs that we should strive to make these work together.

Fortunately, I have a suggestion for code changes that will allow .showHelpByDefault() to still work with .strict() mode configured, where strict mode applies to everything except the magic command (custom default commands are still incompatible).

The resulting behavior is that help content is displayed until a runnable command is specified, at which point strict mode kicks in and errors on any unknown arguments or options at the innermost command that was tried. Hopefully that strikes a good balance for CLIs that are responsive/forgiving for unknown/unspecified commands and yet strict for known/specified commands.

I'll post my suggestions in a review soon.

[nexdrew]

Overall, your approach is great! I just have some minor tweaks to support a couple more scenarios.

Let me know what you think. Thanks!

[elliot-nelson]

Thanks for the detailed feedback!

• Incorporated all the suggested changes and new test cases.
• Slightly tweaked your suggestion on newChild (it now takes an optional options hash to support future options, but has the same effect as your example).

I also realized after looking at your example that if a user does want "all of my commands behave strictly but then I have a fall-back that is not strict", you can do that:

sywac
  .strict()
  .command(require('command1'))
  .command(require('command2'))
  .command(require('command3'))
  .command({
    aliases: '*',
    setup: sywac => {
      sywac.strict(false)
    },
    run: argv => {
      console.log(`You tried to ${argv._[0]}, here's a bunny instead!`)
    }
  })

This could be documented in sywac-website, or possibly we could hack the types/Command constructor to force strictMode to false if the alias matches *. (I say this because I can't imagine any possible scenario where an explicit * command is useful with strict mode on: it's impossible for it to run. So in that case forcing it to false and documenting that strict mode isn't supported by the default command fallback could make sense.)

[nexdrew]

Slightly tweaked your suggestion on newChild (it now takes an optional options hash to support future options, but has the same effect as your example).

Good call!

possibly we could hack the types/Command constructor to force strictMode to false if the alias matches *. (I say this because I can't imagine any possible scenario where an explicit * command is useful with strict mode on: it's impossible for it to run. So in that case forcing it to false and documenting that strict mode isn't supported by the default command fallback could make sense.)

I like this idea, but can we experiment with that in a follow-up PR? I'd like to go ahead and merge this as is.

We should also document .strict() mode in sywac-website on the sync-config page. It should have a <sup>Since 1.3.0</sup> note since I plan to release this as version 1.3.0 soon.