Closed Bug 1057694 Opened 5 years ago Closed 5 years ago

Commands suggestion in mach

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: akachkach, Assigned: akachkach)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Attachment #8477826 - Flags: feedback?(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."
Attachment #8477826 - Flags: feedback?(gps) → feedback+
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).
Attachment #8477826 - Attachment is obsolete: true
Attachment #8478537 - Flags: review?(gps)
Blocks: 1058375
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?
Attachment #8478537 - Flags: review?(gps) → review+
(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.
Keywords: checkin-needed
Code comments documenting that reasoning would be very helpful.
Sure! Will update the patch.
Keywords: checkin-needed
carry r+ forward;
(added comments)
Attachment #8478537 - Attachment is obsolete: true
Attachment #8479439 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/041030a16bae
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.