-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-68583: webbrowser: replace getopt
with argparse
, add long options
#117047
Conversation
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 did not look at the new CliTest class. I have not use argparse but have read doc and examples and it looks good, and much nicer than existing. One suggestion.
When you're done making the requested changes, leave the comment: |
|
||
url = args[0] | ||
open(url, new_win) | ||
def parse_args(arg_list: list[str] | None): |
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.
What's our current policy on stdlib type hints?
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.
python/devguide#1304 says in general don't add, but there are some specific exceptions, but that the policy is undocumented. I think these might be okay because they're "simple" and internal, but I'm also happy to remove if you'd prefer?
"https://example.com -n -t", | ||
"https://example.com --new-window --new-tab", | ||
"https://example.com -n --new-tab", | ||
"https://example.com --new-window -t", |
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.
argparse
allows argument shortening and handles ambiguous shortenings as one might hope, but it might be nice to confirm that --new
fails properly.
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.
Nice, I didn't know about that feature :) I've added a test case.
Replace getopt with argparse in webbrowser's CLI.
Also add long options for
-n
and-t
:--new-window
and--new-tab
.Add tests for the CLI (inspired by https://pythontest.com/testing-argparse-apps/), and for an error case of
new()
.Before
After
Plus some cleanup:
📚 Documentation preview 📚: https://cpython-previews--117047.org.readthedocs.build/