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.

Sign up for GitHub

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

Jump to bottom

bpo-45235: Fix argparse overrides namespace with subparser defaults #28420

Merged
merged 2 commits into from
Sep 18, 2021
Merged

bpo-45235: Fix argparse overrides namespace with subparser defaults #28420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b320a40
bpo-45235: Fix argparse overrides namespace with subparser defaults
ALSchwalm Sep 17, 2021
4182953
📜🤖 Added by blurb_it.
blurb-it[bot] Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions Lib/argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,7 +1209,8 @@ def __call__(self, parser, namespace, values, option_string=None):
# namespace for the relevant parts.
subnamespace, arg_strings = parser.parse_known_args(arg_strings, None)
for key, value in vars(subnamespace).items():
setattr(namespace, key, value)
if not hasattr(namespace, key):
setattr(namespace, key, value)

if arg_strings:
vars(namespace).setdefault(_UNRECOGNIZED_ARGS_ATTR, [])
Expand Down Expand Up @@ -1843,11 +1844,6 @@ def parse_known_args(self, args=None, namespace=None):
if action.default is not SUPPRESS:
setattr(namespace, action.dest, action.default)

# add any parser defaults that aren't present
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this block need to be moved below? This change is breaking traitlets (and IPython) who rely on there being a default value set before the Action logic fires in self._parse_known_args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to move setting the defaults to after the actions because otherwise subparser defaults will be hidden by the parent's defaults (e.g., test_set_defaults_on_parent_and_subparser will fail). It's not obvious to me how that could be avoided. Perhaps the defaults could be exposed in some other way for libraries that depend on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

Maybe all that is needed is documentation that relying on the defaults to be there before the Actions run is broken. I am pretty sure that the old behavior was not intentional and that you used to have access to mutable default values in the Actions was a weird implementation detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change on when default value is set also breaks azure-cli: Azure/azure-cli#20269 (comment)

for dest in self._defaults:
if not hasattr(namespace, dest):
setattr(namespace, dest, self._defaults[dest])

# parse the arguments and exit if there are any errors
if self.exit_on_error:
try:
Expand All @@ -1858,6 +1854,11 @@ def parse_known_args(self, args=None, namespace=None):
else:
namespace, args = self._parse_known_args(args, namespace)

# add any parser defaults that aren't present
for dest in self._defaults:
if not hasattr(namespace, dest):
setattr(namespace, dest, self._defaults[dest])

if hasattr(namespace, _UNRECOGNIZED_ARGS_ATTR):
args.extend(getattr(namespace, _UNRECOGNIZED_ARGS_ATTR))
delattr(namespace, _UNRECOGNIZED_ARGS_ATTR)
Expand Down
6 changes: 6 additions & 0 deletions Lib/test/test_argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3079,6 +3079,12 @@ def test_set_defaults_on_parent_and_subparser(self):
xparser.set_defaults(foo=2)
self.assertEqual(NS(foo=2), parser.parse_args(['X']))

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)))

def test_set_defaults_same_as_add_argument(self):
parser = ErrorRaisingArgumentParser()
parser.set_defaults(w='W', x='X', y='Y', z='Z')
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2021-09-17-16-55-37.bpo-45235.sXnmPA.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix an issue where argparse would not preserve values in a provided namespace
when using a subparser with defaults.