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: Fix argparse overrides namespace with subparser defaults #28420

Merged

Conversation

@ALSchwalm
Copy link
Contributor

@ALSchwalm ALSchwalm commented Sep 17, 2021

argparse will now preserve existing values in a provided namespace
even if a subparser has a default for a value in the namespace. For
example:

import argparse
parser = argparse.ArgumentParser()
sub = parser.add_subparsers()
example_subparser = sub.add_parser("example")
example_subparser.add_argument("--flag", default=10)
print(parser.parse_args(["example"], argparse.Namespace(flag=20)))

This snippet now returns 'Namespace(flag=20)' instead of
'Namespace(flag=10)'.

https://bugs.python.org/issue45235

argparse will now preserve existing values in a provided namespace
even if a subparser has a default for a value in the namespace. For
example:

    import argparse
    parser = argparse.ArgumentParser()
    sub = parser.add_subparsers()
    example_subparser = sub.add_parser("example")
    example_subparser.add_argument("--flag", default=10)
    print(parser.parse_args(["example"], argparse.Namespace(flag=20)))

This snippet now returns 'Namespace(flag=20)' instead of
'Namespace(flag=10)'.
@the-knights-who-say-ni
Copy link

@the-knights-who-say-ni the-knights-who-say-ni commented Sep 17, 2021

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

CLA Missing

Our records indicate the following people have not signed the CLA:

@ALSchwalm

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

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

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

@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Sep 17, 2021

This looks good.

Please add a Misc/NEWS entry with blurb.

@ALSchwalm
Copy link
Contributor Author

@ALSchwalm ALSchwalm commented Sep 17, 2021

Done, I have also signed the CLA, it's just not showing up yet.

@rhettinger rhettinger merged commit a6e8db5 into python:main Sep 18, 2021
12 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Sep 18, 2021

Thanks @ALSchwalm for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒🤖 I'm not a witch! I'm not a witch!

miss-islington added a commit to miss-islington/cpython that referenced this issue Sep 18, 2021
…ythonGH-28420)

(cherry picked from commit a6e8db5)

Co-authored-by: Adam Schwalm <adamschwalm@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Sep 18, 2021

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

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Sep 18, 2021

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

miss-islington added a commit to miss-islington/cpython that referenced this issue Sep 18, 2021
…ythonGH-28420)

(cherry picked from commit a6e8db5)

Co-authored-by: Adam Schwalm <adamschwalm@gmail.com>
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Sep 18, 2021

Thanks @ALSchwalm for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Sep 18, 2021

Thanks @ALSchwalm for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒🤖

miss-islington added a commit to miss-islington/cpython that referenced this issue Sep 18, 2021
…ythonGH-28420)

(cherry picked from commit a6e8db5)

Co-authored-by: Adam Schwalm <adamschwalm@gmail.com>
miss-islington added a commit to miss-islington/cpython that referenced this issue Sep 18, 2021
…ythonGH-28420)

(cherry picked from commit a6e8db5)

Co-authored-by: Adam Schwalm <adamschwalm@gmail.com>
@@ -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

@tacaswell tacaswell Sep 27, 2021

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

@ALSchwalm ALSchwalm Sep 27, 2021

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

@tacaswell tacaswell Sep 28, 2021

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

@jiasli jiasli Nov 15, 2021

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)

tacaswell added a commit to tacaswell/traitlets that referenced this issue Sep 27, 2021
In python/cpython#28420 the order of setting the
default values on the Namespace object and processing the Actions.  This means
that in the `__call__` method of `_FlagAction` the `_flags` attribute has not
yet been put on the namespace.

This change makes `_FlagAction.__call__` forgiving if the `_flags` attribute
does not exist (by creating it!).
tacaswell added a commit to tacaswell/traitlets that referenced this issue Sep 27, 2021
In python/cpython#28420 the order of setting the
default values on the Namespace object and processing the Actions was reversed.
This means that in the `__call__` method of `_FlagAction` the `_flags`
attribute has not yet been put on the namespace.

This change makes `_FlagAction.__call__` forgiving if the `_flags` attribute
does not exist (by creating it!).
@jiasli
Copy link

@jiasli jiasli commented Nov 10, 2021

Unfortunately, this causes a breaking change and backward compatibility issue as detailed in

rhettinger added a commit to rhettinger/cpython that referenced this issue Nov 11, 2021
rhettinger added a commit that referenced this issue Nov 12, 2021
* Revert "bpo-45235: Fix argparse overrides namespace with subparser defaults (GH-28420) (GH-28443)"

This reverts commit a18d522.
miss-islington added a commit to miss-islington/cpython that referenced this issue 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 added a commit to miss-islington/cpython that referenced this issue 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>
remykarem added a commit to remykarem/cpython that referenced this issue Dec 7, 2021
…GH-29525)

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

This reverts commit a18d522.
remykarem added a commit to remykarem/cpython that referenced this issue 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
Projects
None yet
7 participants