GeezerVison

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

GeezerVison is showing github.com — bpo-29298: Fix crash for required subparsers in argparse by BenSokol · Pull Request #18564 · python/cpython

Conversation

BenSokol commented Feb 20, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

When an instance of subparsers is has required=True, and no subparser is found,
an uncaught exception was thrown.

OS: Windows - Cygwin
Python Version: 3.8.0b4
This issue still appears to be present in master

Fixes the following stack trace:
...
File "/usr/lib/python3.8/argparse.py", line 1760, in parse_args
args, argv = self.parse_known_args(args, namespace)
File "/usr/lib/python3.8/argparse.py", line 1792, in parse_known_args
namespace, args = self._parse_known_args(args, namespace)
File "/usr/lib/python3.8/argparse.py", line 2027, in _parse_known_args
', '.join(required_actions))
TypeError: sequence item 0: expected str instance, NoneType found

https://bugs.python.org/issue29298

Sorry, something went wrong.

Uh oh!

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

All reactions


          Fix crash for required subparsers in argparse

b64faa6

When an instance of subparsers is has required=True, and no subparser is found,
an uncaught exception was thrown.

the-knights-who-say-ni commented Feb 20, 2020

Copy link

Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@BenSokol

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

All reactions

Sorry, something went wrong.

Uh oh!

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

the-knights-who-say-ni added the CLA not signed label

Feb 20, 2020

bedevere-bot added the awaiting review label

Feb 20, 2020


          Create 2020-02-20-01-53-01.bpo-29298.laIn2S.rst

b5b458b

codecov Bot commented Feb 20, 2020 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Codecov Report

Merging #18564 into master will increase coverage by 0.98%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #18564       +/-   ##
===========================================
+ Coverage   82.15%   83.13%    +0.98%     
===========================================
  Files        1958     1571      -387     
  Lines      589837   414808   -175029     
  Branches    44458    44459        +1     
===========================================
- Hits       484560   344844   -139716     
+ Misses      95636    60337    -35299     
+ Partials     9641     9627       -14

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-12.86%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Lib/dbm/__init__.py	`66.66% <0.00%> (-4.45%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.87%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
... and 428 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2ee21d...3b4e42b. Read the comment docs.

All reactions

Sorry, something went wrong.

Uh oh!

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

the-knights-who-say-ni added CLA signed and removed CLA not signed labels

Feb 21, 2020

BenSokol requested review from aeros, corona10 and matrixise

February 21, 2020 00:23

aeros reviewed

Feb 21, 2020

View reviewed changes

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

Thanks for the PR and welcome @BenSokol!

It looks like another open PR that addresses this issue (or another part of it) in #3680 is currently open and is not inactive (last comment from author 25 days ago). Does this crash still occur with their patch? At a glance, it looks like they address separate parts of the same BPO issue, but I'd like to be certain.

In the meantime, could you also include a new unit test for this in Lib/test/test_argparse.py, to ensure that the fix functions as intended? This is typically needed for most PRs that make code changes in the standard library, particularly for bug fixes or ones that implement some new functionality. It helps to ensure that the fix works in the first place, and in the long term ensure that the issue remains fixed (to avoid regressions as much as possible).

Sorry, something went wrong.

Uh oh!

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

matrixise reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Misc/NEWS.d/next/Library/2020-02-20-01-53-01.bpo-29298.laIn2S.rst Resolved

Uh oh!

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


          Update Misc/NEWS.d/next/Library/2020-02-20-01-53-01.bpo-29298.laIn2S.rst

3b4e42b

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

aeros added the type-bug An unexpected behavior, bug, or error label

Feb 21, 2020

matrixise commented Feb 26, 2020

Copy link

Copy Markdown

Member

Hi @BenSokol

As @aeros said, could you add a unittest?

Thank you

All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel reviewed

May 19, 2021

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2020-02-20-01-53-01.bpo-29298.laIn2S.rst

		@@ -0,0 +1,2 @@
		Fix crash in :mod:`argparse` when using subparsers and its required attribute

iritkatriel May 19, 2021

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.

creash means segfault, not unhandled exception.

Sorry, something went wrong.

Uh oh!

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

All reactions

encukou commented Jul 27, 2021

Copy link

Copy Markdown

Member

This was fixed in GH-3680, which includes tests.

All reactions

Sorry, something went wrong.

Uh oh!

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

encukou closed this

Jul 27, 2021

Reviewers

iritkatriel

iritkatriel left review comments

corona10

Awaiting requested review from corona10

matrixise

Awaiting requested review from matrixise

+1 more reviewer

aeros

aeros left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review type-bug

An unexpected behavior, bug, or error

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.

7 participants

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.

GeezerVison

Conversation

BenSokol commented Feb 20, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 20, 2020

Recognized GitHub username

Uh oh!

codecov Bot commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matrixise commented Feb 26, 2020

Uh oh!

iritkatriel May 19, 2021

Choose a reason for hiding this comment

Uh oh!

encukou commented Jul 27, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

BenSokol commented Feb 20, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

codecov Bot commented Feb 20, 2020 •

edited

Loading

Uh oh!

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