gh-97848: argparse: Disallow misbehaving actions for positional arguments #98367

kkarbowiak · 2022-10-17T20:14:47Z

The disallowed actions are: 'store_true', 'store_false' and 'store_const', which cause a counterintuitive behaviour.

Tests for new behaviour are included.

Issue: argparse: Strange behaviour of positional arguments with store_true, store_false, or store_const action #97848

bedevere-bot · 2022-10-17T20:14:52Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

ghost · 2022-10-17T20:15:18Z

All commit authors signed the Contributor License Agreement.

sobolevn

Thanks you for your proposal!

Technically this is a breaking change. But, in practice I highly doubt that anyone is using it.

Lib/argparse.py

Misc/NEWS.d/next/Library/2022-10-17-20-20-18.gh-issue-97848.RnkhKZ.rst

sobolevn · 2022-10-18T07:57:45Z

Retriggering CI to fix CLA status.

…khKZ.rst Co-authored-by: Nikita Sobolev <[email protected]>

kkarbowiak · 2022-10-21T08:47:03Z

Hi, is there anything else I need to complete to get this merged?

sobolevn

Python is run by volunteers, so reviews by core-devs can take some time :)

But, it is fine to ping folks once in a while if there's no activity.

Lib/argparse.py

kkarbowiak · 2022-10-21T09:16:24Z

Python is run by volunteers, so reviews by core-devs can take some time :)

But, it is fine to ping folks once in a while if there's no activity.

I get it and did not mean to be rude or impatient, I'm just keen to add a tiny contribution :-)

Thank you for the reviews!

kkarbowiak · 2022-11-13T22:01:24Z

@sobolevn Since you have been kind enough to review this and provide valuable cues, could you please have a look once more?

sobolevn

I think this makes sense for 3.12

I will ping @JelleZijlstra as well :)

JelleZijlstra

I think I'll leave this for another core dev.

JelleZijlstra · 2022-11-16T03:48:36Z

Lib/argparse.py

@@ -1540,6 +1540,13 @@ def _get_positional_kwargs(self, dest, **kwargs):
            msg = _("'required' is an invalid argument for positionals")
            raise TypeError(msg)

+        # make sure 'store_true', 'store_false', or 'store_const' action


This feels inelegant because we're hardcoding the names of individual actions. I'm not familiar enough with argparse to propose an alternative implementation though.

@JelleZijlstra thank you for the review.

You say the change feels inelegant, because the names of individual actions are hardcoded. Do I understand correctly that you would prefer some named values or enumeration? Or did you mean something else?

I also am not a huge fan of using these strings directly, but this is the interface exposed by the ArgumentParser class and as such I think the names are committed to and won't change.

Anyway, I am open to all suggestions on how to improve this PR.

I think that more elegant and general solution would be

either add an optional boolean parameter for registry()

or add an optional class attribute in _StoreTrueAction etc.

It can be tested in add_argument(), after testing that the action is a callable.

Actually, such flag already exists. You can check if nargs is 0 to detect actions that do not consume arguments.

kkarbowiak · 2023-01-18T15:20:55Z

@rhettinger Could you please have a look and review this PR? @JelleZijlstra did have a look and decided to pass it some other core dev, but has not assigned anyone else.

Thanks.

serhiy-storchaka · 2024-09-22T06:22:49Z

Lib/argparse.py

@@ -1540,6 +1540,13 @@ def _get_positional_kwargs(self, dest, **kwargs):
            msg = _("'required' is an invalid argument for positionals")
            raise TypeError(msg)

+        # make sure 'store_true', 'store_false', or 'store_const' action


I think that more elegant and general solution would be

either add an optional boolean parameter for registry()

or add an optional class attribute in _StoreTrueAction etc.

It can be tested in add_argument(), after testing that the action is a callable.

serhiy-storchaka · 2024-09-22T06:23:49Z

Lib/argparse.py

+        # is not specified
+        action = kwargs.get('action')
+        if action in ('store_true', 'store_false', 'store_const'):
+            msg = _("{action} is an invalid action for positionals", {'action': action})


The argument of _() should be a string, not a tuple. I afraid that your tests are passed because a wrong TypeError is raised. You should test also the error message to be sure that this is the correct error.

And these error messages should not be internationalized, as they are programming errors, not user input errors.

Thank you for the review and comments. I will get back to this PR later this week.

Actually, I already created a PR with more general solution: #124839.

bedevere-app · 2024-09-22T06:26:44Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

kkarbowiak added 5 commits October 17, 2022 21:22

Disallow 'store_true' action for positional arguments

a6da57d

Disallow 'store_false' action for positional arguments

d8be724

Disallow 'store_const' action for positional arguments

3772f00

Refactor code

b43e81c

Add tests

6644aab

bedevere-bot added the awaiting review label Oct 17, 2022

📜🤖 Added by blurb_it.

a0e7b90

sobolevn reviewed Oct 18, 2022

View reviewed changes

Lib/argparse.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2022-10-17-20-20-18.gh-issue-97848.RnkhKZ.rst Outdated Show resolved Hide resolved

sobolevn closed this Oct 18, 2022

sobolevn reopened this Oct 18, 2022

kkarbowiak and others added 2 commits October 18, 2022 10:24

Refactor code

f07104c

Update Misc/NEWS.d/next/Library/2022-10-17-20-20-18.gh-issue-97848.Rn…

b26fdb4

…khKZ.rst Co-authored-by: Nikita Sobolev <[email protected]>

sobolevn reviewed Oct 21, 2022

View reviewed changes

Lib/argparse.py Outdated Show resolved Hide resolved

Avoid double search

a4597b1

sobolevn approved these changes Nov 13, 2022

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Nov 13, 2022

AlexWaygood changed the title ~~gh-97848: Disallow misbehaving actions for positional arguments~~ gh-97848: argparse: Disallow misbehaving actions for positional arguments Nov 13, 2022

bedevere-bot mentioned this pull request Nov 13, 2022

argparse: Strange behaviour of positional arguments with store_true, store_false, or store_const action #97848

Closed

JelleZijlstra self-requested a review November 13, 2022 22:12

JelleZijlstra reviewed Nov 16, 2022

View reviewed changes

serhiy-storchaka requested changes Sep 22, 2024

View reviewed changes

bedevere-app bot removed the awaiting core review label Sep 22, 2024

bedevere-app bot added the awaiting changes label Sep 22, 2024

serhiy-storchaka closed this Oct 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-97848: argparse: Disallow misbehaving actions for positional arguments #98367

gh-97848: argparse: Disallow misbehaving actions for positional arguments #98367

kkarbowiak commented Oct 17, 2022 •

edited by bedevere-bot

Loading

bedevere-bot commented Oct 17, 2022

ghost commented Oct 17, 2022 •

edited by ghost

Loading

sobolevn left a comment

sobolevn commented Oct 18, 2022

kkarbowiak commented Oct 21, 2022

sobolevn left a comment

kkarbowiak commented Oct 21, 2022

kkarbowiak commented Nov 13, 2022

sobolevn left a comment

JelleZijlstra left a comment

JelleZijlstra Nov 16, 2022

kkarbowiak Nov 29, 2022

serhiy-storchaka Sep 22, 2024

serhiy-storchaka Sep 23, 2024

kkarbowiak commented Jan 18, 2023

serhiy-storchaka Sep 22, 2024

serhiy-storchaka Sep 22, 2024

serhiy-storchaka Oct 1, 2024

kkarbowiak Oct 1, 2024

serhiy-storchaka Oct 1, 2024

bedevere-app bot commented Sep 22, 2024

gh-97848: argparse: Disallow misbehaving actions for positional arguments #98367

gh-97848: argparse: Disallow misbehaving actions for positional arguments #98367

Conversation

kkarbowiak commented Oct 17, 2022 • edited by bedevere-bot Loading

bedevere-bot commented Oct 17, 2022

ghost commented Oct 17, 2022 • edited by ghost Loading

sobolevn left a comment

Choose a reason for hiding this comment

sobolevn commented Oct 18, 2022

kkarbowiak commented Oct 21, 2022

sobolevn left a comment

Choose a reason for hiding this comment

kkarbowiak commented Oct 21, 2022

kkarbowiak commented Nov 13, 2022

sobolevn left a comment

Choose a reason for hiding this comment

JelleZijlstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkarbowiak commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Sep 22, 2024

kkarbowiak commented Oct 17, 2022 •

edited by bedevere-bot

Loading

ghost commented Oct 17, 2022 •

edited by ghost

Loading