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

bpo-45235: Fix argparse namespace overridden by subparsers default #29574

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+41 −55
Open

bpo-45235: Fix argparse namespace overridden by subparsers default #29574

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
c54e604
bpo-45235: Fix argparse namespace overriden by subparsers default
frostming Nov 16, 2021
71c1e0e
📜🤖 Added by blurb_it.
blurb-it[bot] Nov 16, 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
53 changes: 22 additions & 31 deletions Lib/argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,7 +1022,8 @@ def __init__(self,
metavar=metavar)

def __call__(self, parser, namespace, values, option_string=None):
items = getattr(namespace, self.dest, None)
items = getattr(
namespace, self.dest, self.default if self.default != SUPPRESS else None)
items = _copy_items(items)
items.append(values)
setattr(namespace, self.dest, items)
Expand All @@ -1049,7 +1050,8 @@ def __init__(self,
metavar=metavar)

def __call__(self, parser, namespace, values, option_string=None):
items = getattr(namespace, self.dest, None)
items = getattr(
namespace, self.dest, self.default if self.default != SUPPRESS else None)
items = _copy_items(items)
items.append(self.const)
setattr(namespace, self.dest, items)
Expand Down Expand Up @@ -1205,20 +1207,16 @@ def __call__(self, parser, namespace, values, option_string=None):
# store any unrecognized options on the object, so that the top
# level parser can decide what to do with them

# In case this subparser defines new defaults, we parse them
# in a new namespace object and then update the original
# 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)
_, arg_strings = parser.parse_known_args(arg_strings, namespace)

if arg_strings:
vars(namespace).setdefault(_UNRECOGNIZED_ARGS_ATTR, [])
getattr(namespace, _UNRECOGNIZED_ARGS_ATTR).extend(arg_strings)

class _ExtendAction(_AppendAction):
def __call__(self, parser, namespace, values, option_string=None):
items = getattr(namespace, self.dest, None)
items = getattr(
namespace, self.dest, self.default if self.default != SUPPRESS else None)
items = _copy_items(items)
items.extend(values)
setattr(namespace, self.dest, items)
Expand Down Expand Up @@ -1837,18 +1835,6 @@ def parse_known_args(self, args=None, namespace=None):
if namespace is None:
namespace = Namespace()

# add any action defaults that aren't present
for action in self._actions:
if action.dest is not SUPPRESS:
if not hasattr(namespace, action.dest):
if action.default is not SUPPRESS:
setattr(namespace, action.dest, action.default)

# add any parser defaults that aren't present
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 @@ -1859,6 +1845,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])
Comment on lines +1848 to +1851
Copy link
Contributor

@jiasli jiasli Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The default value we are referring to in an Action doesn't necessarily come from an argument like --foo. It may come from set_defaults:

            command_parser.set_defaults(
                func=metadata,
                command=command_name,
                _cmd=metadata,

Thus, when calling self._parse_known_args, self._defaults will be in namespace.

image

If these 3 lines are moved down here, self._defaults won't be in namespace when calling self._parse_known_args, resulting in failure

  File "C:\Users\name\AppData\Local\Programs\Python\Python310\lib\argparse.py", line 2003, in consume_optional
    take_action(action, args, option_string)
  File "C:\Users\name\AppData\Local\Programs\Python\Python310\lib\argparse.py", line 1931, in take_action
    action(self, namespace, argument_values, option_string)
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\commands\arm.py", line 357, in __call__
    profile = Profile(cli_ctx=namespace._cmd.cli_ctx)  # pylint: disable=protected-access
AttributeError: 'Namespace' object has no attribute '_cmd'

I am open to whether this is the correct behavior, but it is definitely a BREAKING CHANGE given the behavior has been there for years. If this change is what Python committee want, it should be released in at least Python 3.12, not Python 3.10 or 3.11 which are already stabilized, per an email discussion with @gvanrossum:

Even in 3.11 such a breaking change would not be acceptable. They will have to figure out another way to provide the functionality.

Copy link
Author

Choose a reason for hiding this comment

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

This case makes sense to me. I admit that any change may be a breaking change to some people. This needs more discussion, it perhaps should happen on BPO I guess?

Copy link

Choose a reason for hiding this comment

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

Yes, these are great concerns to get feedback from BPO-45235. Some backward compatibility concerns are already in that thread, and as maintainer of another project that was affected by the reverted change in Python 3.9.8, I'd love to see those get hashed out in the main thread instead of the line notes of this patch.

Copy link
Member

@iritkatriel iritkatriel Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Could you in the meantime submit patches for the unit tests we’re missing?

There are at least two - for the regression that happened and the one that was identified here.

Copy link
Author

Choose a reason for hiding this comment

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

they are included in the one added case, do you suggest to separate into two cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to step away from this one. The last attempt at a fix was catastrophic and should teach us that almost every nuance of the implementation is being relied upon. I think you're correct in saying "any change may be a breaking change to some people. " ISTM that there is no possible benefit that would be worth breaking some long deployed, stable, working command-line tools

Copy link
Author

Choose a reason for hiding this comment

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

In my second attempt #30219 the side effect is removed, it should be cleaner.

The issue itself is a valid one, but the first fix isn't that careful.


if hasattr(namespace, _UNRECOGNIZED_ARGS_ATTR):
args.extend(getattr(namespace, _UNRECOGNIZED_ARGS_ATTR))
delattr(namespace, _UNRECOGNIZED_ARGS_ATTR)
Expand Down Expand Up @@ -2077,16 +2068,16 @@ def consume_positionals(start_index):
if action.required:
required_actions.append(_get_action_name(action))
else:
# Convert action default now instead of doing it before
# parsing arguments to avoid calling convert functions
# twice (which may fail) if the argument was given, but
# only if it was defined already in the namespace
if (action.default is not None and
isinstance(action.default, str) and
hasattr(namespace, action.dest) and
action.default is getattr(namespace, action.dest)):
setattr(namespace, action.dest,
self._get_value(action, action.default))
# Set the default value if the arg is not present after
# all arguments are parsed. This ensures that default values
# from subparsers override correctly
if (action.dest != SUPPRESS and
not hasattr(namespace, action.dest) and
action.default != SUPPRESS):
default_value = action.default
if isinstance(default_value, str):
default_value = self._get_value(action, action.default)
setattr(namespace, action.dest, default_value)

if required_actions:
self.error(_('the following arguments are required: %s') %
Expand Down
42 changes: 18 additions & 24 deletions Lib/test/test_argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1838,17 +1838,6 @@ def __call__(self, parser, namespace, value, option_string=None):
# check destination and option string
assert self.dest == 'spam', 'dest: %s' % self.dest
assert option_string == '-s', 'flag: %s' % option_string
# when option is before argument, badger=2, and when
# option is after argument, badger=<whatever was set>
expected_ns = NS(spam=0.25)
if value in [0.125, 0.625]:
expected_ns.badger = 2
elif value in [2.0]:
expected_ns.badger = 84
else:
raise AssertionError('value: %s' % value)
assert expected_ns == namespace, ('expected %s, got %s' %
(expected_ns, namespace))
except AssertionError:
e = sys.exc_info()[1]
raise ArgumentParserError('opt_action failed: %s' % e)
Expand All @@ -1862,19 +1851,6 @@ def __call__(self, parser, namespace, value, option_string=None):
option_string)
# check destination
assert self.dest == 'badger', 'dest: %s' % self.dest
# when argument is before option, spam=0.25, and when
# option is after argument, spam=<whatever was set>
expected_ns = NS(badger=2)
if value in [42, 84]:
expected_ns.spam = 0.25
elif value in [1]:
expected_ns.spam = 0.625
elif value in [2]:
expected_ns.spam = 0.125
else:
raise AssertionError('value: %s' % value)
assert expected_ns == namespace, ('expected %s, got %s' %
(expected_ns, namespace))
except AssertionError:
e = sys.exc_info()[1]
raise ArgumentParserError('arg_action failed: %s' % e)
Expand Down Expand Up @@ -1986,6 +1962,16 @@ def _get_parser(self, subparser_help=False, prefix_chars=None,

# return the main parser
return parser

def _get_parser_with_shared_option(self):
parser = ErrorRaisingArgumentParser(prog='PROG', description='main description')
parser.add_argument('-f', '--foo', default='0')
subparsers = parser.add_subparsers()
parser1 = subparsers.add_parser('1')
parser1.add_argument('-f', '--foo', default='1')
parser2 = subparsers.add_parser('2')
parser2.add_argument('-f', '--foo', default='2')
return parser

def setUp(self):
super().setUp()
Expand Down Expand Up @@ -2341,6 +2327,14 @@ def test_alias_help(self):
3 3 help
"""))

def test_subparsers_with_shared_option(self):
parser = self._get_parser_with_shared_option()
self.assertEqual(parser.parse_args([]), NS(foo='0'))
self.assertEqual(parser.parse_args(['1']), NS(foo='1'))
self.assertEqual(parser.parse_args(['2']), NS(foo='2'))
self.assertEqual(parser.parse_args(['-f', '10', '1', '-f', '42']), NS(foo='42'))
self.assertEqual(parser.parse_args(['1'], NS(foo='42')), NS(foo='42'))

# ============
# Groups tests
# ============
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2021-11-16-08-55-25.bpo-45235.OV9_9i.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix an issue that subparsers defaults override the existing values in the argparse Namespace.