python/cpythonPublic

NotificationsYou must be signed in to change notification settings
Fork34.1k
Star71.6k

Comments

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

Closed

abdulrafey38 wants to merge 24 commits intopython:mainfrom

abdulrafey38:fix-issue-99749

Closed

gh-99749: Add closest choice if exists in Argparser if wrong choice picked#99773
abdulrafey38 wants to merge 24 commits intopython:mainfrom
abdulrafey38:fix-issue-99749

Conversation

Copy link

abdulrafey38 commentedNov 25, 2022•
edited
Loading

Description

Add closet choice if exists in Argparser if wrong choice picked

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

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 argparsechoices argument#99749

Closed

bedevere-bot added the awaiting review label

Nov 25, 2022

Copy link

bedevere-bot commentedNov 25, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Copy link

ghost commentedNov 25, 2022•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

blurb-itbotand others added3 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 added11 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 added2 commits

November 25, 2022 23:37

test cases fixed 🔨

6eeae5c

assert fix

9965694

abdulrafey38 force-pushed thefix-issue-99749 branch from3416d13 to9965694Compare

November 25, 2022 19:10

abdulrafey38 added4 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

Copy link

Contributor

noamcohen97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Lib/argparse.py Outdated

Comment on lines 2551 to 2552

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

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

Copy link

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why not the above one liner?

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

the one liner is less readable - at least to me

Lib/argparse.py Outdated

		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)

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this might fail ifaction.choices are not strings

Copy link

Author

abdulrafey38Nov 26, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 suggested changes

Nov 26, 2022

View reviewed changes

Copy link

NIKDISSV-Forever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 marked this pull request as ready for review

November 26, 2022 13:25

abdulrafey38 requested review fromsobolevn and removed request forNIKDISSV-Forever

November 26, 2022 13:25

Copy link

Author

abdulrafey38 commentedNov 28, 2022•
edited
Loading

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

abdulrafey38 closed this

Nov 28, 2022

Copy link

Member

sobolevn commentedNov 28, 2022

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.

Copy link

Author

abdulrafey38 commentedNov 28, 2022

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

abdulrafey38 reopened this

Nov 28, 2022

Copy link

Author

abdulrafey38 commentedNov 28, 2022

@sobolevn reopened this PR again, please have a review

Copy link

Member

sobolevn commentedNov 28, 2022

Quoting myself:

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

Copy link

Author

abdulrafey38 commentedNov 28, 2022

Quoting myself:
Isn'tinvalid 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'tinvalid choice: %(value)r (choose from %(choices)s)) it now throws msg including closest_choice which is (Isn'tinvalid choice: %(value)r, may be you meant %(closest_choice)r (choose from %(choices)s).

Copy link

Author

abdulrafey38 commentedNov 30, 2022

@sobolevn do we have any update here?

rhettinger self-assigned this

Dec 3, 2022

Copy link

Contributor

rhettinger commentedDec 3, 2022

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

We've tried hard to not add dependencies toargparse because startup time is important in command line applications. The import ofdifflib 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.

Copy link

Member

encukou commentedMar 19, 2024

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.

Copy link

Author

abdulrafey38 commentedMar 19, 2024

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

Copy link

Member

encukou commentedMar 20, 2024

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

savannahostrowski requested changes

Sep 23, 2024

View reviewed changes

Copy link

Member

savannahostrowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Lib/argparse.py

		]


		import difflib as _difflib

Copy link

Member

savannahostrowskiSep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Lib/test/test_argparse.py

		with self.assertRaises(ArgumentParserError) as excinfo:
		parser.parse_args(('baz',))
		self.assertRegex(
		self.assertIn(

Copy link

Member

savannahostrowskiSep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We probably also want some additional test cases here.

Lib/test/test_argparse.py

		parser.parse_args(('baz',))
		self.assertRegex(
		self.assertIn(
		"error: argument {foo,bar}: invalid choice: 'baz', maybe you meant 'bar'? (choose from 'foo', 'bar')",

Copy link

Member

savannahostrowskiSep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.