GeezerVison

View reviewed changes

aeros left a comment •

edited

Loading

Uh oh!

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

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.

Thanks for the PR @shihai1991! I could definitely see the addition of an exit_on_error argument being quite useful to give users more customization.

I would recommend the following additions:

Update the docstring by adding the following on line 1626 (in Lib/argparse.py):

- exit_on_error -- Determines whether or not argparser exits when an error occurs.

Update the docs for ArgumentParser.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Misc/NEWS.d/next/Library/2019-08-21-16-38-56.bpo-9938.t3G7N9.rst Resolved

Uh oh!

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

bedevere-bot added awaiting core review and removed awaiting review labels

aeros added the type-feature A feature request or enhancement label

aeros commented Aug 22, 2019 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Also, I would recommend adjusting the PR title to [bpo-9938](https://bugs.python.org/issue9938): Add optional kwarg to ArgParser. This will allow core devs to more easily identify what the PR is addressing.

All reactions

Sorry, something went wrong.

Uh oh!

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

aeros commented Aug 22, 2019

Copy link

Copy Markdown

Contributor

The Travis CI failure can be fixed by locally running make patchcheck to address the whitespace issue. The problem seems to be occurring from Lib/test/test_argparse.py.

All reactions

Sorry, something went wrong.

Uh oh!

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


          update desc

07336d5

shihai1991 commented Aug 22, 2019 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member Author

Thanks for the PR @shihai1991! I could definitely see the addition of an exit_on_error argument being quite useful to give users more customization.

I would recommend the following additions:

Update the docstring by adding the following on line 1626 (in Lib/argparse.py):
- exit_on_error -- Determines whether or not argparser exits when an error occurs.
Update the docs for ArgumentParser.

Looks Determines whether or not argparser exits with error when an error occurs would better. ;)

exit_on_error -- Determines whether or not argparser exits when an error occurs.

np.

aeros 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.

shihai1991 changed the title ~~bpo-9938: Improve interactive use of argparse~~ bpo-9938: Add optional kwargs to argparse

shihai1991 commented Aug 22, 2019

Copy link

Copy Markdown

Member Author

Also, I would recommend adjusting the PR title to [bpo-9938](https://bugs.python.org/issue9938): Add optional kwarg to ArgParser. This will allow core devs to more easily identify what the PR is addressing.

the desc looks good to me. But why the title need add the bpo link now? the format of title have been changed?

All reactions

Sorry, something went wrong.

Uh oh!

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

aeros approved these changes

View reviewed changes

aeros left a comment •

edited

Loading

Uh oh!

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

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.

@shihai1991

the desc looks good to me. But why the title need add the bpo link now? the format of title have been changed?

I may have been a bit unclear, apologies. I just included the bpo part so that you wouldn't accidentally remove it when editing the title and I didn't intend to include the link. It looks good now.

Thanks for making the recommended changes.

I have a couple of minor grammar suggestions, but once those are added, everything documentation-related looks good as far as I can tell. (:

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/argparse.py Resolved

Uh oh!

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

Comment thread

Doc/library/argparse.rst Resolved

Uh oh!

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

Comment thread

Doc/library/argparse.rst Resolved

Uh oh!

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

shihai1991 and others added 2 commits

August 23, 2019 12:49


          Update Lib/argparse.py

8cb0b26

Co-Authored-By: Kyle Stanley <[email protected]>


          Update Doc/library/argparse.rst

a163c4a

Co-Authored-By: Kyle Stanley <[email protected]>

shihai1991 commented Aug 23, 2019

Copy link

Copy Markdown

Member Author

@shihai1991

the desc looks good to me. But why the title need add the bpo link now? the format of title have been changed?

I may have been a bit unclear, apologies. I just included the bpo part so that you wouldn't accidentally remove it when editing the title and I didn't intend to include the link. It looks good now.

Thanks for making the recommended changes.

I have a couple of minor grammar suggestions, but once those are added, everything documentation-related looks good as far as I can tell. (:

thanks, good job :)

aeros 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.

shihai1991 added 3 commits

August 24, 2019 00:58


          add argument examples

bdecd79


          update argument's desc

1eadaee


          update detail of colon

25924d1

shihai1991 commented Aug 23, 2019

Copy link

Copy Markdown

Member Author

cc @rhettinger

All reactions

Sorry, something went wrong.

Uh oh!

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

aeros reviewed

Aug 24, 2019

View reviewed changes

Comment thread

Doc/library/argparse.rst Resolved

Uh oh!

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

aeros reviewed

Aug 24, 2019

View reviewed changes

Comment thread

Doc/library/argparse.rst Resolved

Uh oh!

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

shihai1991 and others added 2 commits

August 24, 2019 10:23


          Update Doc/library/argparse.rst

872b47a

Co-Authored-By: Kyle Stanley <[email protected]>


          Update Doc/library/argparse.rst

facfb4c

Co-Authored-By: Kyle Stanley <[email protected]>

aeros approved these changes

Aug 24, 2019

View reviewed changes

matrixise self-assigned this

matrixise changed the title ~~bpo-9938: Add optional kwargs to argparse~~ bpo-9938: Add optional keyword argument exit_on_error to argparse.ArgumentParser

matrixise suggested changes

View reviewed changes

matrixise 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.

Thank you for your PR, please update with my remarks.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Doc/library/argparse.rst Resolved

Uh oh!

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

Comment thread

Doc/library/argparse.rst Resolved

Uh oh!

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

Comment thread

Lib/argparse.py Resolved

Uh oh!

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

bedevere-bot added awaiting changes and removed awaiting core review labels

bedevere-bot commented Sep 11, 2019

Copy link

Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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


          update desc of exit_on_error in argparse

aee9372

shihai1991 commented Sep 12, 2019

Copy link

Copy Markdown

Member Author

I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting change review and removed awaiting changes labels

bedevere-bot commented Sep 12, 2019

Copy link

Copy Markdown

Thanks for making the requested changes!

@matrixise: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot requested a review from matrixise

September 12, 2019 00:34

matrixise suggested changes

View reviewed changes

Comment thread

Doc/library/argparse.rst Outdated

		abbreviation is unambiguous. (default: ``True``)

		* exit_on_error_ - Determines whether or not ArgumentParser exits with
		error info when an error occur. (default: ``True``)

matrixise Sep 12, 2019

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.

when an error occurs

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 Sep 12, 2019

Copy link

Copy Markdown

Member 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.

ops, thank you :)

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot commented Sep 12, 2019

Copy link

Copy Markdown

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting changes and removed awaiting change review labels


          Fix typo

03451ed

matrixise approved these changes

View reviewed changes

matrixise 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.

Thank you for your PR, I have fixed one Typo, but for the rest, I agree.

Sorry, something went wrong.

Uh oh!

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

aeros reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-bot added awaiting merge and removed awaiting changes labels

matrixise added the 🤖 automerge label

miss-islington commented Sep 12, 2019

Copy link

Copy Markdown

Contributor

@shihai1991: Status check is done, and it's a success ✅ .

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington merged commit f545638 into python:master

bedevere-bot removed the awaiting merge label

DinoV pushed a commit to DinoV/cpython that referenced this pull request