GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-99749: Add closest choice if exists in Argparser if wrong choice picked by abdulrafey38 · Pull Request #99773 · python/cpython

Conversation

@abdulrafey38

@abdulrafey38 abdulrafey38 commented Nov 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Description

Add closet choice if exists in Argparser if wrong choice picked

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
noamcohen97 reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@abdulrafey38
Add closet choice if exists in argparser if wrong choice picked
ef8e4fc
@bedevere-bot bedevere-bot mentioned this pull request Nov 25, 2022
Add “maybe you meant” in argparse choices argument #99749
Closed
@bedevere-bot bedevere-bot added the awaiting review label Nov 25, 2022
@bedevere-bot

bedevere-bot commented Nov 25, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost

ghost commented Nov 25, 2022 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
blurb-it Bot and others added 3 commits November 25, 2022 12:23
@blurb-it
📜🤖 Added by blurb_it.
7e8dcf8
@abdulrafey38
Fix documentation
8c754cd
@abdulrafey38
Added EOL 🔨
4fb8ce6
@AlexWaygood 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
@abdulrafey38
Test cases updated for argparser
2f197b3
@abdulrafey38
Fixed typo errors 🔨
940a66e
@abdulrafey38
Test case fix 🔨
badc5ed
@abdulrafey38
Fixed test cse error msg 🔨
2588ef1
@abdulrafey38
Fixed error test case assert msg 🔨
ee05c1e
@abdulrafey38
assertion fix 🔨
4a36406
@abdulrafey38
assertion test case fix
fe169bc
@abdulrafey38
Test Case fix
9fe0d95
@abdulrafey38
Reveet to assertRegex from assertEqual
d12e1c4
@abdulrafey38
Remove unused imports 🔨
35f0961
@abdulrafey38
assertion msg fixed 🔨
087895c
@abdulrafey38
abdulrafey38 marked this pull request as draft November 25, 2022 18:12
abdulrafey38 added 2 commits November 25, 2022 23:37
@abdulrafey38
test cases fixed 🔨
6eeae5c
@abdulrafey38
assert fix
9965694
@abdulrafey38
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
@abdulrafey38
assertion fix
4ef218a
@abdulrafey38
revert testing
e63a01c
@abdulrafey38
test: hammer:
bfc9262
@abdulrafey38
fixed
b3b4a9e
noamcohen97
noamcohen97 reviewed Nov 26, 2022
View reviewed changes

@noamcohen97 noamcohen97 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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

@noamcohen97 noamcohen97 Nov 25, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@abdulrafey38 abdulrafey38 Nov 26, 2022

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@noamcohen97 noamcohen97 Nov 26, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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)

@noamcohen97 noamcohen97 Nov 26, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this might fail if action.choices are not strings

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@abdulrafey38 abdulrafey38 Nov 26, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@noamcohen97 noamcohen97 Nov 26, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
abdulrafey38 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
ndenissov
ndenissov suggested changes Nov 26, 2022
View reviewed changes

@ndenissov ndenissov left a comment

Copy link
Copy Markdown

There was a problem hiding this 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

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 = []

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
noamcohen97 and abdulrafey38 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@abdulrafey38
abdulrafey38 marked this pull request as ready for review November 26, 2022 13:25
@abdulrafey38
abdulrafey38 requested review from sobolevn and removed request for ndenissov November 26, 2022 13:25
@abdulrafey38

abdulrafey38 commented Nov 28, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdulrafey38 abdulrafey38 closed this Nov 28, 2022
@sobolevn

sobolevn commented Nov 28, 2022

Copy link
Copy Markdown
Member

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.
noamcohen97 reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdulrafey38

abdulrafey38 commented Nov 28, 2022

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdulrafey38 abdulrafey38 reopened this Nov 28, 2022
@abdulrafey38

abdulrafey38 commented Nov 28, 2022

Copy link
Copy Markdown
Author

@sobolevn reopened this PR again, please have a review
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sobolevn

sobolevn commented Nov 28, 2022

Copy link
Copy Markdown
Member

Quoting myself:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdulrafey38

abdulrafey38 commented Nov 28, 2022

Copy link
Copy Markdown
Author

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdulrafey38

abdulrafey38 commented Nov 30, 2022

Copy link
Copy Markdown
Author

@sobolevn do we have any update here?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rhettinger rhettinger self-assigned this Dec 3, 2022
@rhettinger

rhettinger commented Dec 3, 2022

Copy link
Copy Markdown
Contributor

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.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Mar 19, 2024

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdulrafey38

abdulrafey38 commented Mar 19, 2024

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Mar 20, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
savannahostrowski
savannahostrowski requested changes Sep 23, 2024
View reviewed changes

@savannahostrowski savannahostrowski left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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!))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/argparse.py
]


import difflib as _difflib

@savannahostrowski savannahostrowski Sep 23, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_argparse.py
with self.assertRaises(ArgumentParserError) as excinfo:
parser.parse_args(('baz',))
self.assertRegex(
self.assertIn(

@savannahostrowski savannahostrowski Sep 23, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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')",

@savannahostrowski savannahostrowski Sep 23, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@abdulrafey38

abdulrafey38 commented Sep 24, 2024

Copy link
Copy Markdown
Author

Hi @savannahostrowski ,

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

Thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 24, 2024
@savannahostrowski
Merge branch 'main' into pythongh-99773
2b802ec
@savannahostrowski savannahostrowski mentioned this pull request Sep 24, 2024
GH-99749: Add optional feature to suggest correct names (ArgumentParser) #124456
Merged
@savannahostrowski

savannahostrowski commented Sep 24, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@savannahostrowski savannahostrowski closed this Sep 24, 2024
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 24, 2024
@savannahostrowski
Merge branch 'main' into pythongh-99773
2c39866
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 24, 2024
@savannahostrowski
Merge branch 'main' into pythongh-99773
fb5f041
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 12, 2024
@savannahostrowski
Merge branch 'main' into pythongh-99773
d2c665d
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 12, 2024
@savannahostrowski
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
@serhiy-storchaka
Merge branch 'main' into pythongh-99773
f2f67e7
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 17, 2024
@savannahostrowski
Merge branch 'main' into pythongh-99773
c494469
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 17, 2024
@savannahostrowski
Merge branch 'pythongh-99773' of https://github.com/savannahostrowski…
68b9383 
…/cpython into pythongh-99773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@savannahostrowski savannahostrowski savannahostrowski requested changes
@sobolevn sobolevn Awaiting requested review from sobolevn
+2 more reviewers
@noamcohen97 noamcohen97 noamcohen97 left review comments
@ndenissov ndenissov ndenissov requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@rhettinger rhettinger

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@abdulrafey38 @bedevere-bot @sobolevn @rhettinger @encukou @savannahostrowski @noamcohen97 @ndenissov
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.