gh-99749: Add closest choice if exists in Argparser if wrong choice picked by abdulrafey38 · Pull Request #99773 · python/cpython

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

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


Suggested change
ifclosest_choice:=closest_choiceandclosest_choice[0] orNone:
args['closest'] =closest_choice
ifclosest_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

- 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)

ndenissov

Please, instead of just this

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

Add

ifisinstance(value, Iterable) andall(isinstance(option, Sized) foroptioninaction.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)
exceptTypeError:
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

 ]

-
+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

 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

 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-itBotand 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 pickedNov 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 9965694Compare 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

ndenissov 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 ndenissov 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' intopythongh-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' intopythongh-99773
2c39866

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 24, 2024
Merge branch 'main' intopythongh-99773
fb5f041

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 12, 2024
Merge branch 'main' intopythongh-99773
d2c665d

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 12, 2024
Merge branch 'pythongh-99773' ofhttps://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' intopythongh-99773
f2f67e7

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 17, 2024
Merge branch 'main' intopythongh-99773
c494469

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 17, 2024
Merge branch 'pythongh-99773' ofhttps://github.com/savannahostrowski…
68b9383
…/cpython into pythongh-99773

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

-ifclosest_choice:=closest_choiceandclosest_choice[0] orNone:
-args['closest'] =closest_choice
+ifclosest_choice:
+args['closest'] =closest_choice[0]

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

Uh oh!

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!

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

abdulrafey38 commented Nov 25, 2022•
edited
Loading

ghost commented Nov 25, 2022•
edited by ghost
Loading

abdulrafey38Nov 26, 2022•
edited
Loading

abdulrafey38 commented Nov 28, 2022•
edited
Loading