Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adds recommendCommands() for command suggestions #580

Merged
merged 11 commits into from Aug 14, 2016

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Aug 7, 2016

working from @maxrimue's branch for recommendCommands(), and implementing @nexdrew's code-review.

This pull request gets us to v1 of command recommendations (thanks @maxrimue for the hard work) addressing some of the nits in #562 (@maxrimue sorry for @nexdrew and I providing such a difficult review).

Here's a breakdown of some of the changes made:

  • rather than pulling in another dependency, I simply added a helper module for calculating levenshtein distance: this is a fairly simple algorithm, and I could imagine us using it in other places -- might as well dance around adding another dependency to yargs.
  • we now handle the edge-case that @nexdrew outlined with nested arguments significantly better, by avoiding making recommendations until after we've walked through all the command handlers.
  • we now recommend commands using the validation module, this allows us to piggyback off existing functionality in usage.js, and validation.js.
  • we now use the same method signature as strict() for recommendCommands() rather than trying to be overly fancy with argument overriding.

reviewers: @maxrimue @nexdrew I plan on stubbornly marching us towards yargs@5.x over the next week, appreciate your patience.

@maxrimue maxrimue mentioned this pull request Aug 7, 2016
10 tasks
@bcoe
Copy link
Member Author

bcoe commented Aug 9, 2016

TODO: sort by length before applying algorithm, so that we take the least-changes/longest word first.

@@ -707,6 +713,10 @@ function Yargs (processArgs, cwd, parentRequire) {
}
}

// recommend a command if recommendCommands() has
// been enabled, and no commands were found to execute
if (recommendCommands && argv._.length) validation.recommendCommands(argv._[argv._.length - 1], handlerKeys)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is called even when argv._ only contains valid commands or arguments (called as part of running the command); it just potentially doesn't have any commands to match against.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argv._.length check is actually redundant.

@nexdrew
Copy link
Member

nexdrew commented Aug 10, 2016

I would probably change lines 706 - 718 to this:

      // if there's a handler associated with a
      // command defer processing to it.
      var handlerKeys = command.getCommands()
      if (handlerKeys.length) {
        var firstUnknownCommand
        for (var i = 0, cmd; (cmd = argv._[i]) !== undefined; i++) {
          if (~handlerKeys.indexOf(cmd) && cmd !== completionCommand) {
            setPlaceholderKeys(argv)
            return command.runCommand(cmd, self, parsed)
          } else if (!firstUnknownCommand && cmd !== completionCommand) {
            firstUnknownCommand = cmd
          }
        }

        // recommend a command if recommendCommands() has
        // been enabled, and no commands were found to execute
        if (recommendCommands && firstUnknownCommand) {
          validation.recommendCommands(firstUnknownCommand, handlerKeys)
        }
      }

What do you think?

@nexdrew nexdrew added the 5.x label Aug 13, 2016
@bcoe bcoe merged commit 59474dc into master Aug 14, 2016
@bcoe bcoe deleted the merged-recommend-similiar-commands branch August 14, 2016 01:45
@nexdrew nexdrew mentioned this pull request Oct 10, 2016
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants