Skip to content
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

bpo-45235: Revert an argparse bugfix that caused a regression #29525

Merged

Conversation

rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Nov 11, 2021

@rhettinger rhettinger added type-bug An unexpected behavior, bug, or error skip news needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Nov 11, 2021
@rhettinger rhettinger requested a review from ambv November 11, 2021 18:53
Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

We could remove this testing blindspot if we also added a test for the case that broke with GH-28420.

Comment on lines -3117 to -3122
def test_set_defaults_on_subparser_with_namespace(self):
parser = argparse.ArgumentParser()
xparser = parser.add_subparsers().add_parser('X')
xparser.set_defaults(foo=1)
self.assertEqual(NS(foo=2), parser.parse_args(['X'], NS(foo=2)))

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we keep this test and mark is as xfail until we bring back a better patch for the original issue?

Copy link
Contributor Author

@rhettinger rhettinger Nov 11, 2021

Choose a reason for hiding this comment

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

I think it would be better to leave it out. Also, any future changes will likely only go onto the main branch, so there is no value in leaving disabled tests in older versions. I would like to just do a clean revert. The open BPO issue will serve as the marker for future work to be done.

@miss-islington
Copy link
Contributor

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-29530 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Nov 12, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 12, 2021
…GH-29525)

* Revert "bpo-45235: Fix argparse overrides namespace with subparser defaults (pythonGH-28420) (pythonGH-28443)"

This reverts commit a18d522.
(cherry picked from commit 807f839)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 12, 2021
…GH-29525)

* Revert "bpo-45235: Fix argparse overrides namespace with subparser defaults (pythonGH-28420) (pythonGH-28443)"

This reverts commit a18d522.
(cherry picked from commit 807f839)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@bedevere-bot
Copy link

GH-29531 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Nov 12, 2021
@@ -0,0 +1,3 @@
Reverted an argparse bugfix that caused regression in the handling of
default arguments for subparsers. This prevented leaf level arguments from
Copy link
Contributor

Choose a reason for hiding this comment

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

The word "This" seems a little bit ambiguous to me. It can mean this PR (#29525) or the bugfix/regression.

remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
…GH-29525)

* Revert "bpo-45235: Fix argparse overrides namespace with subparser defaults (pythonGH-28420) (pythonGH-28443)"

This reverts commit a18d522.
remykarem pushed a commit to remykarem/cpython that referenced this pull request Jan 30, 2022
…GH-29525)

* Revert "bpo-45235: Fix argparse overrides namespace with subparser defaults (pythonGH-28420) (pythonGH-28443)"

This reverts commit a18d522.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants