1057694 - Commands suggestion in mach

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Ahmed Kachkach [:akachkach]]

This could be a nice addition to mach :) (a comment indicates that this was planned in the future).

Example:

    ➜  gecko-dev git:(command-suggestion) ./mach moctest-plai
    It looks like you are trying to run an unknown mach command: moctest-plai

    Did you want to run any of these commands: mochitest-plain, mochitest-a11y, mochitest?

    Run |mach help| to show a list of commands.

Running non-sense won't suggest an arbitrary command:

    ➜  gecko-dev git:(command-suggestion) ./mach fdiofjdiosj
    It looks like you are trying to run an unknown mach command: fdiofjdiosj

    Run |mach help| to show a list of commands.


This uses difflib, so no external dependency or fancy algorithm needed.

Comment 1

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1057694-Command-suggestions-in-mach-r-gps.patch

Here's a proposed patch, extending the UnknownCommandError exception with an optional suggested_commands argument and using difflib to get some suggestions.

Slightly changed the output to take into account the case where we have a single suggestion:

➜  gecko-dev git:(command-suggestion) ./mach test-plain
It looks like you are trying to run an unknown mach command: test-plain

Did you want to run this mach command instead: mochitest-plain?

Run |mach help| to show a list of commands.


And:


➜  gecko-dev git:(command-suggestion) ./mach mochitest-
It looks like you are trying to run an unknown mach command: mochitest-

Did you want to run any of these commands instead: mochitest, mochitest-a11y, mochitest-plain?

Run |mach help| to show a list of commands.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8477826 [details] [diff] [review]
0001-Bug-1057694-Command-suggestions-in-mach-r-gps.patch

Review of attachment 8477826 [details] [diff] [review]:
-----------------------------------------------------------------

Love the idea!

We'd definitely want tests for this, especially if we go the automatic remapping route.

::: python/mach/mach/dispatcher.py
@@ +111,3 @@
>          if not handler:
> +            suggested_commands = difflib.get_close_matches(command, self._mach_registrar.command_handlers.keys())
> +            raise UnknownCommandError(command, 'run', suggested_commands)

Hmm. I'm actually not sure what algorithm difflib.get_close_matches() uses under the hood.

If I were writing this algorithm from scratch, I would:

1) Look for string prefix matches. If found and not ambiguous, use it.
2) Compute Levenshtein distance. Use closest match if distance <= N, where N is likely proportional to the length of the characters typed or matching command.

::: python/mach/mach/main.py
@@ +397,5 @@
>              return 1
>          except UnknownCommandError as e:
> +            if e.suggested_commands:
> +                if len(e.suggested_commands) == 1:
> +                    suggestion_message = SUGGESTED_SINGLE_COMMAND_MESSAGE % (e.verb, e.suggested_commands[0])

I think that if we have one match and confidence is high, we should just execute that command. e.g.

"I'm assuming the 'foo' command is 'foobar' and am executing it for you."

Comment 3

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1057694-Command-suggestions-in-mach-r-gps.patch

This implements "command remapping" and also adds commands that begin with the given command to the list of suggestions ("mochitest-" will suggest all commands beginning with "mochitest-", like autocompletion in bash would do).

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8478537 [details] [diff] [review]
0001-Bug-1057694-Command-suggestions-in-mach-r-gps.patch

Review of attachment 8478537 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good enough to land.

::: python/mach/mach/dispatcher.py
@@ +109,5 @@
> +        # Command suggestion
> +        if command not in self._mach_registrar.command_handlers:
> +            suggested_commands = difflib.get_close_matches(command, self._mach_registrar.command_handlers.keys(), cutoff=0.8)
> +            if len(suggested_commands) != 1:
> +                suggested_commands = set(difflib.get_close_matches(command, self._mach_registrar.command_handlers.keys(), cutoff=0.5))

This seems slightly weird that we're expanding the search after len != 1. What if len > 1? Is the original cutoff not acceptable?

Comment 5

[Ahmed Kachkach [:akachkach]]

(In reply to Gregory Szorc [:gps] from comment #4)

Thanks for the review!

> Comment on attachment 8478537 [details] [diff] [review]
> 0001-Bug-1057694-Command-suggestions-in-mach-r-gps.patch
> 
> Review of attachment 8478537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good enough to land.
> 
> ::: python/mach/mach/dispatcher.py
> @@ +109,5 @@
> > +        # Command suggestion
> > +        if command not in self._mach_registrar.command_handlers:
> > +            suggested_commands = difflib.get_close_matches(command, self._mach_registrar.command_handlers.keys(), cutoff=0.8)
> > +            if len(suggested_commands) != 1:
> > +                suggested_commands = set(difflib.get_close_matches(command, self._mach_registrar.command_handlers.keys(), cutoff=0.5))
> 
> This seems slightly weird that we're expanding the search after len != 1.
> What if len > 1? Is the original cutoff not acceptable?


The thing is that we want a big accuracy for automatic remapping, that's why the cutoff value is higher here. If we have 0 suggestion or many suggestions, we'll still want all "recommended" commands: since we couldn't find out ONE really good command to execute, we want all the possible good-ish suggestions.

An example is with "mochtest-": there's more than one suggestion so we can't do automatic remapping, but not all possible/recommended suggestion.

Comment 6

[Gregory Szorc [:gps]]

Code comments documenting that reasoning would be very helpful.

Comment 7

[Ahmed Kachkach [:akachkach]]

Sure! Will update the patch.

Comment 8

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1057694-Command-suggestions-in-mach-r-gps.patch

carry r+ forward;
(added comments)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/041030a16bae

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/041030a16bae