python / cpython

The Python programming language
https://www.python.org/
Other
61.16k stars 29.52k forks source link

argparse: TypeError: __init__() takes at least 4 arguments (4 given) #69486

Closed 0f0f0158-5183-401f-ab06-166278473c4d closed 2 years ago

0f0f0158-5183-401f-ab06-166278473c4d commented 8 years ago
BPO 25299
Nosy @bitdancer, @phmc, @tyomitch, @nanjekyejoannah, @iritkatriel

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: ```python assignee = None closed_at = created_at = labels = ['extension-modules', 'type-bug', 'docs'] title = 'argparse: TypeError: __init__() takes at least 4 arguments (4 given)' updated_at = user = 'https://github.com/tyomitch' ``` bugs.python.org fields: ```python activity = actor = 'iritkatriel' assignee = 'docs@python' closed = True closed_date = closer = 'iritkatriel' components = ['Documentation', 'Extension Modules'] creation = creator = 'A. Skrobov' dependencies = [] files = [] hgrepos = [] issue_num = 25299 keywords = ['3.5regression'] message_count = 17.0 messages = ['252106', '252120', '252121', '252123', '252124', '252127', '252133', '252135', '252192', '252240', '349553', '349584', '349878', '349899', '349910', '349912', '408401'] nosy_count = 7.0 nosy_names = ['r.david.murray', 'docs@python', 'paul.j3', 'pconnell', 'A. Skrobov', 'nanjekyejoannah', 'iritkatriel'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue25299' versions = ['Python 2.7', 'Python 3.5'] ```

0f0f0158-5183-401f-ab06-166278473c4d commented 8 years ago
Python 2.7.3 (default, Dec 18 2014, 19:10:20) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from argparse import ArgumentParser
>>> parser = ArgumentParser()
>>> parser.add_argument("--foo", help="foo", action='store_const')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/argparse.py", line 1281, in add_argument
    action = action_class(**kwargs)
TypeError: __init__() takes at least 4 arguments (4 given)
>>> 

First, the exception message is entirely unhelpful (you wanted at least 4, you got 4, what's your problem?)

Second, adding "const=None" to the invocation makes it succeed; but, according to https://docs.python.org/2/library/argparse.html#const this argument defaults to "None", so it shouldn't be necessary to specify it explicitly. Thus, either the documentation or the implementation is wrong.

bitdancer commented 8 years ago

Yes, the python2 TypeErrors for mimatched arguments are sub-optimal. This has been fixed in python3:

>>> parser.add_argument('--foo', help="foo", action='store_const')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/rdmurray/python/p36/Lib/argparse.py", line 1336, in add_argument
    action = action_class(**kwargs)
TypeError: __init__() missing 1 required positional argument: 'const'
bitdancer commented 8 years ago

Oops, sorry, I missed the fact that you were pointing out a doc issue.

bitdancer commented 8 years ago

Hmm. After looking at the code, it may also be desirable to improve that error message at the argparse level, since there's a bit of not-immediately-obvious indirection going on there.

0f0f0158-5183-401f-ab06-166278473c4d commented 8 years ago

Thank you for confirming that the mismatch between the documentation and the behaviour is preserved in Python 3!

Adding it to the list of affected versions.

bitdancer commented 8 years ago

To clarify this for other readers: the issue here is that the arguments to add_argument are passed through the action routine, which in this case is store_const, and store_const is the one that requires 'const' be specified...which isn't made clear by either the docs or the error message.

7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 8 years ago

A related issue is

http://bugs.python.org/issue24754 argparse add_argument with action="store_true", type=bool should not crash

In this case the user specified a parameter that the 'store_true' subclass did not accept.

In both cases, 'parser.addargument' does minimal error checking (or catching), so the user ends up seeing the '\_init__' argument error.

This line in the 'store_const' documentation is clearly wrong. There's no default.

(Note that the const keyword argument defaults to the rather
unhelpful None.)

Possible fixes to the bigger issue:

bitdancer commented 8 years ago

I think your second to last choice (catch the error) is the only practical solution. It is also probably the best, since we need to support user-written action routines.

7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 8 years ago

A fix that I am exploring would wrap the Action instantiation call in add_argument with a try/except block

That is replace:

    def add_argument(...)
        ....
        action = action_class(**kwargs)
        ...

with

    ....
    try:
        action = action_class(**kwargs)
    except TypeError:
        msg = str(_sys.exc_info()[1])
        msg = msg.replace('__init__','add_argument')
        msg = [msg]
        msg.append('Wrong argument(s) for Action subclass: %s'%action_class.__name__)
        # action_class is now a class, rather than the input string
        msg.append('kwargs: %s'%kwargs)
        sig = getattr(action_class, 'action_sig',None)
        if sig is not None:
            msg = sig(msg)
        raise ValueError(msg)
   ....

This collects information on the error, the action class and arguments, and passes them to a class method of the Action. That customizes the message, and returns it for reraising.

In 'class Action' I define:

    @classmethod
    def action_sig(cls, msg=[]):
        # return the signature
        # subclasses may return custom version
        import inspect
        try:
            name = cls.__name__
            sig = inspect.signature(cls.__init__)
            sig = str(sig)
        except AttributeError:
            spec = inspect.getfullargspec(cls.__init__)
            sig = inspect.formatargspec(*spec)
        # remove self, option_strings, dest
        dstr='dest,'
        ind = sig.find(dstr)
        if ind>=0:
            sig = '(...'+sig[(ind+len(dstr)+1):]
        sig = 'class args: %s'%sig
        msg.append(sig)
        return '\n'.join(msg)

This adds inspect.signature information from the subclass __init__ method to the message from add_argument. Subclasses (including the user defined ones) could customize this.

So a missing 'const' error would display as:

ValueError: add_argument() missing 1 required positional argument: 'const' Wrong argument(s) for Action subclass: _StoreConstAction kwargs: {'option_strings': ['--bar'], 'dest': 'bar'} class args: (...const, default=None, required=False, help=None, metavar=None)

This may be overkill, but it gives an idea of the information that we can add to the original TypeError.

Done right this action_sig() could also serve as a usage function for an Action class.

7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 8 years ago

The unittest file test_argparse.py tests add_argument parameters in

    class TestInvalidArgumentConstructors(TestCase)

It looks at a dozen different categories of errors, mostly checking that they return the correct TypeError or ValueError. It does not check the content of error messages.

If I change the code I described yesterday to return a TypeError again (but with an enhanced message), it passes this unittest.

Most of the test_argparse.py tests just check for error type. There just a couple of late additions that check error message content:

TestMessageContentError
TestAddArgumentMetavar
nanjekyejoannah commented 4 years ago

paul.j3,

The fix looks promising. Do you want to open a PR with this fix?

7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 4 years ago

I'm not set up to work with the current development distribution (via github). All my proposed patches are diffs for earlier repos.

Paul

nanjekyejoannah commented 4 years ago

this argument defaults to "None"

Am leaning to the fact that this works as required. The documentation does not just say defaults to none. It says:

With the 'store_const' and 'append_const' actions, the const keyword argument must be given. For other actions, it defaults to None.

In the scenario, you have given, you are using 'store_const' so the const keyword argument must be given.

This should be closed If you agree with what I have just stated.

0f0f0158-5183-401f-ab06-166278473c4d commented 4 years ago

Joannah, I see that under bpo-25314, the docs were updated to match the implementation: https://github.com/python/cpython/commit/b4912b8ed367e540ee060fe912f841cc764fd293

On the other hand, the discussion here (from 2015) and on bpo-25314 (from 2016) includes suggestions for improvements in the implementation as well.

nanjekyejoannah commented 4 years ago

Since both the docs and implementation now match, I suggest we close this and open a new issue suggesting to change the implementation back to make const=None when action='store_const'.

nanjekyejoannah commented 4 years ago

Opened bpo-37880 to track this change.

iritkatriel commented 2 years ago

This is working on 3.11:

>>> from argparse import ArgumentParser
>>> parser = ArgumentParser()
>>> parser.add_argument("--foo", help="foo", action='store_const')
_StoreConstAction(option_strings=['--foo'], dest='foo', nargs=0, const=None, default=None, type=None, choices=None, help='foo', metavar=None)
>>> parser.print_usage()
usage: [-h] [--foo]
>>> 

So I agree this issue can be closed.