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

Positional arguments with boolean actions behave differently #85935

Open
rjeffman mannequin opened this issue Sep 12, 2020 · 6 comments
Open

Positional arguments with boolean actions behave differently #85935

rjeffman mannequin opened this issue Sep 12, 2020 · 6 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir

Comments

@rjeffman
Copy link
Mannequin

rjeffman mannequin commented Sep 12, 2020

BPO 41769
Nosy @terryjreedy, @rjeffman

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-09-12.02:38:08.047>
labels = ['3.8', 'library']
title = 'Positional arguments with boolean actions behave differently'
updated_at = <Date 2020-09-21.14:45:29.058>
user = 'https://github.com/rjeffman'

bugs.python.org fields:

activity = <Date 2020-09-21.14:45:29.058>
actor = 'rjeffman'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2020-09-12.02:38:08.047>
creator = 'rjeffman'
dependencies = []
files = []
hgrepos = []
issue_num = 41769
keywords = []
message_count = 4.0
messages = ['376764', '377141', '377241', '377258']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'paul.j3', 'rjeffman']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'test needed'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue41769'
versions = ['Python 3.8']

Linked PRs

@rjeffman
Copy link
Mannequin Author

rjeffman mannequin commented Sep 12, 2020

argparse allow the use of store_true and store_false for positional arguments, and although it is weird, it should be fine, but using either action raises a behavior I believe is wrong.

Given the following Python code:

import argparse

arg = argparse.ArgumentParser()
arg.add_argument("opt", action="store_false")

arg.parse_args(["-h"])

The output is:

usage: t.py [-h]

positional arguments:
  opt

optional arguments:
  -h, --help  show this help message and exit

Note that the positional argument is not shown in the usage line.

When any string parameter is given, the result is:

usage: t.py [-h]
t.py: error: unrecognized arguments:

(add to the end of the output the value of the argument.)

Even if the use of a positional value is not the best way to describe boolean parameter (optional arguments provide a much better interface for such values), if argparse is to support positional boolean values they should work as other positional arguments.

I'd suggest raising an error if store_true or store_false is used along with positional values.

@rjeffman rjeffman mannequin added 3.8 only security fixes stdlib Python modules in the Lib dir labels Sep 12, 2020
@terryjreedy
Copy link
Member

You think the behavior is wrong. Does it disagree with the doc? If not, this is an design change (enhancement) issue limited to future version. If so, it might be a bug that can be backported.

However, when I run the code on Windows with an argument
f:\dev\3x>py -3.8 f:/python/a/tem3.py a b
usage: tem3.py [-h]
...

I do not get an error, but get the same usage message as without. Adding quotes on the command line makes no difference.

@terryjreedy terryjreedy changed the title Positional arguments which use store boolean actions do not behave as other actions. Positional arguments with boolean actions behave differently Sep 18, 2020
@terryjreedy terryjreedy changed the title Positional arguments which use store boolean actions do not behave as other actions. Positional arguments with boolean actions behave differently Sep 18, 2020
@paulj3
Copy link
Mannequin

paulj3 mannequin commented Sep 21, 2020

'store_true/false' are subclasses of 'store_const'. All have a 'nargs=0' value. It's that value which explains their behavior. ('append_const' would also fall in this category)

In [2]: parser = argparse.ArgumentParser()
In [3]: parser.add_argument('foo', action='store_const', default='xxx', const='yyy')
Out[3]: _StoreConstAction(option_strings=[], dest='foo', nargs=0, const='yyy', default='xxx', type=None, choices=None, help=None, metavar=None)
In [4]: _.required
Out[4]: True
In [5]: parser.print_help()
usage: ipython3 [-h]

positional arguments:
  foo

optional arguments:
  -h, --help  show this help message and exit

In [6]: parser.parse_args([])
Out[6]: Namespace(foo='yyy')

In [7]: parser.parse_args(['zzz'])
usage: ipython3 [-h]
ipython3: error: unrecognized arguments: zzz

Like '*' and '?' this argument is 'satisfied' with nothing, an empty list of values. For those nargs, 'get_values' takes a the special step of assigning the default. For 'store_const' it's the 'const' that's placed in the namespace. 'store_true' does store True, and 'store_false' does store False.

Providing a string produces an error because there isn't any Action to consume it. With nargs=0, this Action can't use it. Usage is also correct since we can't provide any string to meet this Action's needs.

Technically then the Action behaves correctly - both in usage and what it does. That said, it normally isn't useful, since it will always assign the 'const/True/False' to the namespace. Even without an in depth knowledge of how parsing is done, this should be logically evident (if not obvious).

I don't recall any previous issues like this, though I wouldn't be surprised if there were. I don't recall anything on Stackoverflow either.

I think adding an error would be overkill. It doesn't come up often, and some user might have their own obscure reason for using it.

A note to the docs might be in order, but until a user has encountered this behavior as you have, such a note might be more confusing than useful.

As a general rule, parameter checking in add_argument() is rather loose (and multilayered). While 'action' is used to select the Action subclass, most of the rest are passed on to that Action. The Action __init__ check a few key parameters (for example 'store_const' will complain if we don't provide a 'const'), but accept or ignore the rest.

@rjeffman
Copy link
Mannequin Author

rjeffman mannequin commented Sep 21, 2020

I don't think many users will try to use a boolean positional argument. I only excited this behavior because I was writing an abstraction over argparse to define the command line interface based on a configuration file, and I was trying to add some type checking.

I think documenting the behavior is enough.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@serhiy-storchaka
Copy link
Member

See also #97848,

@serhiy-storchaka
Copy link
Member

There are checks for 'store' and 'append' actions:

        if nargs == 0:
            raise ValueError('nargs for store actions must be != 0; if you '
                             'have nothing to store, actions such as store '
                             'true or store const may be more appropriate')
        if nargs == 0:
            raise ValueError('nargs for append actions must be != 0; if arg '
                             'strings are not supplying the value to append, '
                             'the append const action may be more appropriate')

They were here from the initial release of argparse.

There are also few general checks in add_argument().

So there are precedences. We can either add checks in constructors of BooleanOptionalAction, _StoreConstAction, _AppendConstAction, _CountAction, _HelpAction and _VersionAction, or add a general check in add_argument(). The latter looks easier.

I wonder if there is a case for an "anchor" action. For example, subparsers could have a positional argument with action='store_const' and difference const values, so the stored value would indicate what subparser was executed. But the same goal can be achieved by using dest in add_subparsers() or set_defaults() for subparser.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 1, 2024
Raise ValueError in add_argument() if either explicit nargs=0 or action
that does not consume arguments (like 'store_const' or 'store_true') is
specified for positional argument.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 2, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 2, 2024
serhiy-storchaka added a commit that referenced this issue Oct 2, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 2, 2024
…ythonGH-124891)

Check also specific error messages.
(cherry picked from commit 2c050d4)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 2, 2024
…gparse (pythonGH-124891)

Check also specific error messages.
(cherry picked from commit 2c050d4)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Oct 2, 2024
…GH-124891) (GH-124898)

Check also specific error messages.
(cherry picked from commit 2c050d4)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.8 only security fixes stdlib Python modules in the Lib dir
Projects
Status: No status
Development

No branches or pull requests

2 participants