gh-99749: Add closest choice if exists in Argparser if wrong choice picked #99773

abdulrafey38 · 2022-11-25T12:12:39Z

Description

Add closet choice if exists in Argparser if wrong choice picked

Issue: Add “maybe you meant” in argparse choices argument #99749

bedevere-bot · 2022-11-25T12:12:45Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

ghost · 2022-11-25T12:13:12Z

All commit authors signed the Contributor License Agreement.

noamcohen97

Great feature!
Maybe a way to opt out of this feature should be added?

noamcohen97 · 2022-11-25T23:59:24Z

Lib/argparse.py

+            if closest_choice := closest_choice and closest_choice[0] or None:
+                args['closest'] = closest_choice


Suggested change

if closest_choice := closest_choice and closest_choice[0] or None:

args['closest'] = closest_choice

if closest_choice:

args['closest'] = closest_choice[0]

why not the above one liner?

the one liner is less readable - at least to me

noamcohen97 · 2022-11-26T00:02:29Z

Lib/argparse.py

-            args = {'value': value,
-                    'choices': ', '.join(map(repr, action.choices))}
-            msg = _('invalid choice: %(value)r (choose from %(choices)s)')
+            closest_choice = _difflib.get_close_matches(value, action.choices)


this might fail if action.choices are not strings

argparser does not accept choices other than string, it will throw error even before reaching at this line
reference ==> https://stackoverflow.com/questions/49578928/typeerror-int-object-is-not-subscriptable-when-i-try-to-pass-three-arguments

The second example for the documentation (https://docs.python.org/3/library/argparse.html#choices)

parser = argparse.ArgumentParser(prog='doors.py') parser.add_argument('door', type=int, choices=range(1, 4)) print(parser.parse_args(['3']))

Also - I would use

Suggested change

closest_choice = _difflib.get_close_matches(value, action.choices)

closest_choice = _difflib.get_close_matches(value, action.choices, 1)

NIKDISSV-Forever

Please, instead of just this

closet_choice = _difflib.get_close_matches(value, action.choices)

Add

if isinstance(value, Iterable) and all(isinstance(option, Sized) for option in action.choices)):
    closet_choice = _difflib.get_close_matches(value, action.choices, 1)  # also n=1 (default 3)
else:
    closet_choice = []

or this (better in my opinion)

try:
    closet_choice = _difflib.get_close_matches(value, action.choices, 1)
except TypeError:
    closet_choice = []

abdulrafey38 · 2022-11-28T06:43:41Z

Closing this pull request & opening a new one as this PR contains unwanted commits...

sobolevn · 2022-11-28T07:20:22Z

Please, don't create new PRs.
This PR contains history and reviews. New one is empty. It is better to force push commits in case you have some conflicts than creating a new PR.

abdulrafey38 · 2022-11-28T07:25:12Z

Ok @sobolevn i will open this PR again... and close the new one

abdulrafey38 · 2022-11-28T07:26:57Z

@sobolevn reopened this PR again, please have a review

sobolevn · 2022-11-28T07:41:23Z

Quoting myself:

Isn't invalid choice: %(value)r (choose from %(choices)s) enough?

abdulrafey38 · 2022-11-28T09:33:45Z

Quoting myself:

Isn't invalid choice: %(value)r (choose from %(choices)s) enough?

@sobolevn no this is not enough now, as we change argparser logic here ==> when we pass a choice which is not in choices array it now throws an exception with a different error message (this error message now includes closest suggestions too if available) & in the test case we are passing ['foo','bar'] in choices array and adding 'baz' in argparser. so instead of showing previous message which is (Isn't invalid choice: %(value)r (choose from %(choices)s)) it now throws msg including closest_choice which is (Isn't invalid choice: %(value)r, may be you meant %(closest_choice)r (choose from %(choices)s).

abdulrafey38 · 2022-11-30T08:23:13Z

@sobolevn do we have any update here?

rhettinger · 2022-12-03T18:23:05Z

I'm still evaluating whether this should be done or not. Here are a few thoughts:

We've tried hard to not add dependencies to argparse because startup time is important in command line applications. The import of difflib loads a slew of other modules.
The current choices error message is self-explanatory and we have no evidence that it is insufficient. AFAICT, no end-user has ever asked for this.
If this were added, it would need to be opt-in. There are thousands of deployed command line tools and the publishers of those tools may not want this new behavior.

encukou · 2024-03-19T12:01:36Z

How's the evaluation going?
Some counterpoints:

difflib could be imported on error only
We don't really have a way for end-users to ask; “did you mean” notes for things like AttributeError were generally received positively.
IMO, opt-out would be fine; I don't think the wording of this message is API. But that's a personal opinion.

abdulrafey38 · 2024-03-19T12:21:27Z

@encukou i can work on it further like importing on errors only, what u say?

encukou · 2024-03-20T12:12:34Z

That work might not be merged, so it's better to wait for the decision on whether this is a good idea.

savannahostrowski

Hey there - thanks for the PR! I'm in the process of triaging existing PRs for argparse and noticed this was still open.

A couple of comments but, this still works as intended.

(If you haven't got the bandwidth, let me know and I'd also be happy to carry the PR forward (with credit to you, of course!))

savannahostrowski · 2024-09-23T20:22:16Z

Lib/argparse.py

@@ -84,7 +84,7 @@
    'ZERO_OR_MORE',
 ]

-
+import difflib as _difflib


I think we'd probably want to move the import down into the case where the error is thrown, since this is probably not going to be used very often.

savannahostrowski · 2024-09-23T20:22:37Z

Lib/test/test_argparse.py

@@ -2193,9 +2193,9 @@ def test_wrong_argument_subparsers_no_destination_error(self):
        subparsers.add_parser('bar')
        with self.assertRaises(ArgumentParserError) as excinfo:
            parser.parse_args(('baz',))
-        self.assertRegex(
+        self.assertIn(


We probably also want some additional test cases here.

savannahostrowski · 2024-09-23T22:00:29Z

Lib/test/test_argparse.py

@@ -2193,9 +2193,9 @@ def test_wrong_argument_subparsers_no_destination_error(self):
        subparsers.add_parser('bar')
        with self.assertRaises(ArgumentParserError) as excinfo:
            parser.parse_args(('baz',))
-        self.assertRegex(
+        self.assertIn(
+            "error: argument {foo,bar}: invalid choice: 'baz', maybe you meant 'bar'? (choose from 'foo', 'bar')",


I'd be interested in others' opinions around the verbiage here. It seems like we are using both "Maybe you meant" and "Did you mean" verbiage in the codebase. Not sure if we have any principle around this.

abdulrafey38 · 2024-09-24T09:08:14Z

Hi @savannahostrowski ,

Yes you can pick it and if you need any help do let me know.

Thanks

savannahostrowski · 2024-09-24T18:48:01Z

Carrying this forward in #124456 with @abdulrafey38's blessing. Thank you!

…/cpython into pythongh-99773

Add closet choice if exists in argparser if wrong choice picked

ef8e4fc

bedevere-bot mentioned this pull request Nov 25, 2022

Add “maybe you meant” in argparse choices argument #99749

Closed

bedevere-bot added the awaiting review label Nov 25, 2022

blurb-it bot and others added 3 commits November 25, 2022 12:23

📜🤖 Added by blurb_it.

7e8dcf8

Fix documentation

8c754cd

Added EOL 🔨

4fb8ce6

AlexWaygood changed the title ~~gh-99749: Add closet choice if exists in Argparser if wrong choice picked~~ gh-99749: Add closest choice if exists in Argparser if wrong choice picked Nov 25, 2022

abdulrafey38 added 11 commits November 25, 2022 18:15

Test cases updated for argparser

2f197b3

Fixed typo errors 🔨

940a66e

Test case fix 🔨

badc5ed

Fixed test cse error msg 🔨

2588ef1

Fixed error test case assert msg 🔨

ee05c1e

assertion fix 🔨

4a36406

assertion test case fix

fe169bc

Test Case fix

9fe0d95

Reveet to assertRegex from assertEqual

d12e1c4

Remove unused imports 🔨

35f0961

assertion msg fixed 🔨

087895c

abdulrafey38 marked this pull request as draft November 25, 2022 18:12

abdulrafey38 added 2 commits November 25, 2022 23:37

test cases fixed 🔨

6eeae5c

assert fix

9965694

abdulrafey38 force-pushed the fix-issue-99749 branch from 3416d13 to 9965694 Compare November 25, 2022 19:10

abdulrafey38 added 4 commits November 26, 2022 00:34

assertion fix

4ef218a

revert testing

e63a01c

test: hammer:

bfc9262

fixed

b3b4a9e

noamcohen97 reviewed Nov 26, 2022

View reviewed changes

NIKDISSV-Forever suggested changes Nov 26, 2022

View reviewed changes

abdulrafey38 marked this pull request as ready for review November 26, 2022 13:25

abdulrafey38 requested review from sobolevn and removed request for NIKDISSV-Forever November 26, 2022 13:25

abdulrafey38 closed this Nov 28, 2022

abdulrafey38 reopened this Nov 28, 2022

rhettinger self-assigned this Dec 3, 2022

savannahostrowski requested changes Sep 23, 2024

View reviewed changes

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 24, 2024

Merge branch 'main' into pythongh-99773

2b802ec

savannahostrowski mentioned this pull request Sep 24, 2024

GH-99749: Add optional feature to suggest correct names (ArgumentParser) #124456

Merged

savannahostrowski closed this Sep 24, 2024

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 24, 2024

Merge branch 'main' into pythongh-99773

2c39866

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 24, 2024

Merge branch 'main' into pythongh-99773

fb5f041

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 12, 2024

Merge branch 'main' into pythongh-99773

d2c665d

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 12, 2024

Merge branch 'pythongh-99773' of https://github.com/savannahostrowski…

f57b4dc

…/cpython into pythongh-99773

serhiy-storchaka added a commit to savannahostrowski/cpython that referenced this pull request Oct 14, 2024

Merge branch 'main' into pythongh-99773

f2f67e7

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 17, 2024

Merge branch 'main' into pythongh-99773

c494469

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 17, 2024

Merge branch 'pythongh-99773' of https://github.com/savannahostrowski…

68b9383

…/cpython into pythongh-99773

		if closest_choice := closest_choice and closest_choice[0] or None:
		args['closest'] = closest_choice

	closest_choice = _difflib.get_close_matches(value, action.choices)
	closest_choice = _difflib.get_close_matches(value, action.choices, 1)

Uh oh!

gh-99749: Add closest choice if exists in Argparser if wrong choice picked #99773

gh-99749: Add closest choice if exists in Argparser if wrong choice picked #99773

Uh oh!

Conversation

abdulrafey38 commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

bedevere-bot commented Nov 25, 2022

Uh oh!

ghost commented Nov 25, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noamcohen97 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!

abdulrafey38 Nov 26, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

NIKDISSV-Forever left a comment

Choose a reason for hiding this comment

Uh oh!

abdulrafey38 commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Nov 28, 2022

Uh oh!

abdulrafey38 commented Nov 28, 2022

Uh oh!

abdulrafey38 commented Nov 28, 2022

Uh oh!

sobolevn commented Nov 28, 2022

Uh oh!

abdulrafey38 commented Nov 28, 2022

Uh oh!

abdulrafey38 commented Nov 30, 2022

Uh oh!

rhettinger commented Dec 3, 2022

Uh oh!

encukou commented Mar 19, 2024

Uh oh!

abdulrafey38 commented Mar 19, 2024

Uh oh!

encukou commented Mar 20, 2024

Uh oh!

savannahostrowski 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!

abdulrafey38 commented Sep 24, 2024

Uh oh!

savannahostrowski commented Sep 24, 2024

Uh oh!

Uh oh!

abdulrafey38 commented Nov 25, 2022 •

edited

Loading

ghost commented Nov 25, 2022 •

edited by ghost

Loading

abdulrafey38 Nov 26, 2022 •

edited

Loading

abdulrafey38 commented Nov 28, 2022 •

edited

Loading