Add the ability to autocorrect a user's command #4193

pradyunsg · 2016-12-22T14:05:56Z

Essentially, this adds the ability to:

provide suggestions when the user mistypes
auto-correct the user, if they opt-in, using the best matching command name

pradyunsg · 2016-12-22T17:52:57Z

pip/__init__.py

    # all the args without the subcommand
    cmd_args = args[:]
    cmd_args.remove(cmd_name)

+    # Autocorrect command name
+    if cmd_name not in commands_dict:
+        # MARK: The following should be loaded from the configuration file


This part probably depends on #4192. I'm willing to shoot up another PR (or update this one) once that merges.

pradyunsg · 2016-12-22T17:53:30Z

pip/__init__.py

+        score, guess = get_similar_command(cmd_name)
+
+        if guess is None:  # nothing similar
+            err_msg = 'pip does not have a command "%s"' % cmd_name


I changed the error message. It's nicer IMO.

pradyunsg · 2016-12-22T17:54:50Z

pip/commands/__init__.py

@@ -61,18 +61,42 @@ def get_summaries(ordered=True):
        yield (name, command_class.summary)


-def get_similar_commands(name):
+def get_close_matches(word, possibilities, n=1, cutoff=0.6):


This is a re-implementation that's basically copy-paste removing the last line. Why it's duplication, I don't imagine this code changing anyway. Thoughts?

Implemented a better function for this.

pradyunsg · 2016-12-22T17:56:08Z

pip/commands/__init__.py

    close_commands = get_close_matches(name, commands_dict.keys())

    if close_commands:
        return close_commands[0]
    else:
-        return False
+        return 0, None


This change made life easier when handling results at call-sites.

pradyunsg · 2016-12-22T17:56:33Z

pip/commands/__init__.py

+    return heapq.nlargest(n, result)
+
+
+def get_similar_command(name):


Name change because only one command is returned.

xavfernandez · 2016-12-23T08:34:33Z

pip/__init__.py

+            'Assuming you meant "%s", pip will continue in %.1f seconds...'
+        )
+
+        logger.warn(msg, cmd_name, guess, wait_time)


logger.warning

xavfernandez · 2016-12-23T08:36:28Z

pip/__init__.py

+            # it returns
+            for i in range(int(wait_time * 10)):
+                time.sleep(0.1)
+        except KeyboardInterrupt:


Let the exception bubble up until BaseCommand.

I don't see BaseCommand anywhere in the call stack.

BaseCommand doesn't come into the picture since this is directly called by main to compute which command to initialize. Letting this exception bubble up results in a not-so-nice traceback, since there is no one catching KeyboardInterrupt in a "upper" stack position.

Hmm, you are right. Then, it would be nice to print the same error messages: https://github.com/pypa/pip/blob/bbe99ce/pip/basecommand.py#L236-L237 and sys.exit(pip.status_codes.ERROR), to be consistent.

xavfernandez · 2016-12-23T08:39:59Z

pip/commands/__init__.py

+    return heapq.nlargest(n, result)
+
+
+def get_similar_command(name):
    """Command name auto-correct."""


Maybe update the docstring with the return types

pradyunsg · 2016-12-23T10:22:38Z

This is how it currently looks:

$ pip goooo
ERROR: pip does not have a command "goooo"
$ pip lost
ERROR: pip does not have a command "lost" - did you mean "list"?
$ pip lst
You called a pip command named "lst" which does not exist.
Assuming you meant "list", pip will continue in 2.0 seconds...
^C

xavfernandez · 2016-12-23T11:37:56Z

pip/__init__.py

+        logger.warning(msg, cmd_name, guess, wait_time)
+        try:
+            # time.sleep in a loop because KeyboardInterrupt is raised after
+            # it returns


This seems strange...

$ python -c "import time;time.sleep(60)" ^CTraceback (most recent call last): File "<string>", line 1, in <module> KeyboardInterrupt

works right away...

I sourced it from http://stackoverflow.com/questions/5114292/break-interrupt-a-time-sleep-in-python

The asker ran this on Python 2.2. I didn't notice that. 😅

I'll push the correction.

pradyunsg · 2016-12-25T07:42:12Z

Pinging @dstufft, because he authored #1737.

pradyunsg · 2017-01-14T13:27:33Z

@xavfernandez @dstufft Any outstanding issues with this?

pradyunsg · 2017-02-08T05:34:27Z

Bump.

pradyunsg · 2017-03-13T07:08:51Z

Bump 2.0?

dstufft

Two comments, otherwise looks good.

dstufft · 2017-03-16T19:09:57Z

pip/__init__.py

+            time.sleep(wait_time)
+        except KeyboardInterrupt:
+            logger.critical('Operation cancelled by user')
+            logger.debug('Exception information:', exc_info=True)


We probably don't need to record an exception for a KeyboardInterrupt, I don't think there is any value in the traceback for debugging or anything is there?

Agreed! Will change.

is it really a good idea to just go with the autocorrected command?
this may hide severely broken scripts and problems for longer than necessary,

imho the tool should still fail

I agree. I'm not keen on letting people type the incorrect thing and just carrying on with what we assume they meant. Are there other tools that do this? From what I know, tools that autocorrect tend to either say "did you mean X?" and stop, or say "did you mean X (Y/N)?" and prompt but not proceed without an explicit response from the user.

Having said that, I don't know many tools that do this at all, so I have limited information to work from.

FWIW, I also agree that it's not good to assume what the user meant.

git is one tool that has this behaviour:

$ git coone git: 'coone' is not a git command. See 'git --help'. Did you mean this? clone

Ironically, my git automatically runs the correct command :)

$ git pll WARNING: You called a Git command named 'pll', which does not exist. Continuing under the assumption that you meant 'pull' in 0.1 seconds automatically... Already up-to-date.

Git has a configuration option that can be modified by the user to enable this behavior. help.autocorrect

Since it's been brought up, we could do something like git and hide this behavior behind a configuration option...

If the default is to not assume, then I'm OK with this.

dstufft · 2017-03-16T19:16:19Z

pip/__init__.py

+            )
+            return None
+
+    wait_time = get_float("wait_time", 2.0)


It looks like you're allowing configuration of these values here, but it doesn't appear like you've added the configuration values to the config files, so while it's looking for them, I don't think they'll ever exist.

That is why I have included the fallback values (2.0 for wait_time here).

Is there a better way to have fallbacks for these or am I missing something?

xavfernandez · 2017-03-21T12:36:32Z

It is also now missing a changelog file (cf https://pip.pypa.io/en/latest/development/#adding-a-news-entry)

pradyunsg · 2017-03-21T16:11:45Z

@xavfernandez Will do!

BrownTruck · 2017-04-01T17:05:15Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

dstufft · 2017-04-01T17:38:28Z

Ignore this, Just testing something.

pradyunsg · 2017-04-07T17:39:03Z

I'll come back to this after #4240 - it changes a lot of things regarding the configuration objects.

Those changes make using the configuration easier for this.

BrownTruck · 2017-05-19T06:00:23Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

Because consistency.

pradyunsg · 2017-06-14T10:40:10Z

/request-review @dstufft @pfmoore @xavfernandez

pradyunsg · 2017-06-14T12:24:06Z

With a messy looking recent history, this PR is ready for review. ^.^

pfmoore · 2017-06-14T12:40:51Z

Honestly, I don't like this "do what I mean" type of interface. I'd be OK with a simple message that said "no command foo, did you mean fooo?" and stopped, but the timed wait and automatic running of what we guessed just feels like too much (and I'm speaking as a Windows user, who's supposed to be used to UIs doing what they think I meant 😄)

I get that this is opt-in, so if others support this change then fine, but personally I'm not a fan.

dstufft · 2017-06-17T16:20:38Z

I use it in git where I find it useful because I regularly typo things like git psh or something. I don't feel super strongly though about it, I just thought it might be useful?

pradyunsg · 2017-06-17T19:04:58Z

I use it in git where I find it useful

Same. And, I have mistyped pip insatll multiple times. And it's opt-in, so, I guess this won't hurt?

pradyunsg · 2017-06-19T19:22:48Z

@pfmoore @dstufft Will we be going ahead with this?

pradyunsg · 2017-06-26T18:32:10Z

After a week, ping!

The entire thing has been re-implemented since.

pfmoore · 2017-06-26T20:38:01Z

I won't be merging it myself. But I'm not going to argue if @dstufft does.

pradyunsg · 2017-06-29T14:41:58Z

Ping @dstufft!

dstufft · 2017-07-05T23:54:35Z

I've been thinking about this and talking it over with some friends, and I think ultimately it makes sense to reject this and just close the original issue. The thing that ended up finally tipping the scales into the no camp is that with git, there's pretty much not much you can do that can't be undone, so auto correcting commands is fairly safe. However with pip there's no easy way to undo a command, so guessing wrong has a much higher cost than it does in git.

So primarily for that reason, I'm going to reject this PR. Thanks @pradyunsg for taking a look at this though!

pradyunsg · 2017-07-06T07:28:04Z

I'm going to reject this PR.

No issues. ^.^

Thanks @pradyunsg for taking a look at this though!

You're welcome!!

lock · 2019-06-02T18:06:20Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg commented Dec 22, 2016

View reviewed changes

xavfernandez reviewed Dec 23, 2016

View reviewed changes

pradyunsg force-pushed the autocorrect-command branch from f7ee09a to ffca676 Compare December 25, 2016 07:38

dstufft previously requested changes Mar 16, 2017

View reviewed changes

xavfernandez added this to the 10.0 milestone Mar 16, 2017

BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 1, 2017

dstufft closed this Apr 1, 2017

dstufft reopened this Apr 1, 2017

BrownTruck removed the needs rebase or merge PR has conflicts with current master label Apr 1, 2017

pradyunsg force-pushed the autocorrect-command branch from 1cc216d to 77a5836 Compare April 7, 2017 17:28

pradyunsg mentioned this pull request May 12, 2017

Add a configuration command #4240

Merged

9 tasks

BrownTruck added the needs rebase or merge PR has conflicts with current master label May 19, 2017

pradyunsg changed the title ~~[WIP] Add the ability to autocorrect a user's command~~ Add the ability to autocorrect a user's command Jun 14, 2017

Validate configuration input from the user

70b3bac

pradyunsg force-pushed the autocorrect-command branch from d81d47e to d20ec7d Compare June 14, 2017 07:42

pradyunsg added 4 commits June 14, 2017 13:12

📰

d20ec7d

Sort imports

e9340ed

Because consistency.

import time

6454a7a

Partially undo import sorting

a7cf077

🔧

04e7265

📝

70ea789

pradyunsg added the type: enhancement Improvements to functionality label Jun 26, 2017

pradyunsg self-assigned this Jun 29, 2017

dstufft closed this Jul 5, 2017

dstufft mentioned this pull request Jul 5, 2017

Don't tell the user what they meant, just do what they meant #1737

Closed

pradyunsg deleted the autocorrect-command branch July 6, 2017 07:27

pradyunsg removed their assignment Oct 5, 2017

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

		return heapq.nlargest(n, result)


		def get_similar_command(name):

Add the ability to autocorrect a user's command #4193

Add the ability to autocorrect a user's command #4193

Uh oh!

Conversation

pradyunsg commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradyunsg commented Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradyunsg commented Dec 25, 2016

Uh oh!

pradyunsg commented Jan 14, 2017

Uh oh!

pradyunsg commented Feb 8, 2017

Uh oh!

pradyunsg commented Mar 13, 2017

Uh oh!

dstufft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradyunsg Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavfernandez commented Mar 21, 2017

Uh oh!

pradyunsg commented Mar 21, 2017

pradyunsg commented Dec 22, 2016 •

edited

Loading

pradyunsg commented Dec 23, 2016 •

edited

Loading

pradyunsg Mar 16, 2017 •

edited

Loading

pradyunsg commented Apr 7, 2017 •

edited

Loading