classification
Title: Document the Action API in argparse
Type: behavior Stage:
Components: Documentation Versions: Python 3.4, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: BreamoreBoy, bethard, docs@python, eric.araujo, jaraco, paul.j3, python-dev, terry.reedy, tshepang
Priority: low Keywords: patch

Created on 2011-12-06 14:56 by jaraco, last changed 2014-08-24 05:14 by paul.j3. This issue is now closed.

Files
File name Uploaded Description Edit
956c6d33a57d.diff jaraco, 2011-12-14 04:46 review
b8c09b766e20.diff jaraco, 2014-07-20 15:22 review
Repositories containing patches
http://bitbucket.org/jaraco/cpython-issue13540#2.7
Messages (23)
msg148921 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2011-12-06 14:56
In http://docs.python.org/dev/library/argparse.html#action, when describing an arbitrary action, the documentation states, "You can also specify an arbitrary action by passing an object that implements the Action API." This statement does not link to any documentation on the Action API and does not describe the API except to give an example.

Furthermore, the example contradicts the description. The description says "pass an object" but the example passes a class.

The documentation should clarify the text relating to the example and should document the expected interface for a custom action.
msg149363 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-12-12 22:58
The doc specified the Action API as the interface inherited from argparse.Action plus the addition of a custom __call__ method with 4 params (5 with self) as described. That seems mostly adequate to me, unless there is something missing in the parameter descriptions.

I agree that it should be stated if one should pass the class rather than an instance thereof. (But classes are objects, so that is not a 'contradiction'.) If the example is in error, it should be fixed.
msg149391 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2011-12-13 15:08
You're right. The documentation isn't incorrect, if you're splitting hairs. But it's not super friendly either.

Questions that the documentation should answer:

1) Does the action always need to be a subclass of an Action, or is that recommended? If it's recommended, replace "easiest" with "recommended".
2) If it does not always need to be a subclass of Action, what is the interface expected of a class for a custom action?
3) If one does use a subclass of Action, are there any hooks that are called at any point? How does one customize an action other than to override __call__? What attributes are available on the object (the example shows .dest only)?
4) What is the action required to do in __call__, if anything? Is there any way to invoke the default behavior (if for example a special case wasn't detected)?

All of these questions can be answered by going to the source, but I've twice now gone to the documentation to reference how to provide a custom action and found the documentation insufficient.

Now that I review the source again, I think I know the answers to those questions.

1) It does not always need to be a subclass of Action, but it is recommended.
2) The action API expects a callable with the following signature: Action(option_strings, dest, nargs=None, const=None, default=None, type=None, choices=None, required=False, help=None, metavar=None) which when called returns another callable accepting the four parameters.
3) Argparse does nothing with the custom action except to call it first with the init parameters, then calls the result with four parameters. If subclassing Action, the default __init__ simply stores each of those parameters as attributes on the object. Most subclasses of Action will override the __init__ to validate the options and raise an exception (typically a ValueError) when the parameters aren't valid.
4) The action is not required to do anything. Most actions will perform some logic and then invoke setattr(namespace, self.dest, result) where result is the result of some logic.

I can see why no one wanted to write this documentation. It's difficult to describe simply.

Here's what I propose:

First, remove the phrase "Action API" which suggests that there's an API outside of subclassing the Action that's recommended.

Second, change "easiest" to "recommended" to using the class.

Third, explain what attributes are available on an object initialized from a subclass of Action. Provide a link to code or other section that describes advanced usage (overriding __init__).

Fourth, explain what is expected when calling an Action instance (i.e. setattr).

Fifth, consider extending the Action class so that __call__ calls an actual named method so that those writing custom actions aren't writing classes with only a __call__ method. Perhaps "invoke". I feel less strongly about this point, but it seems strange that the recommended usage is to write a class with only a __call__ method.
msg149422 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2011-12-14 04:44
I've attached a repository and patch with the recommended changes. I created an additional section that includes the documentation for the Action class (specifically the __init__ and __call__ signatures).

I believe this addresses the issues I raised. Any objections to this change? I haven't tested the rendering yet, but I'll do that before pushing to the master if it's adequate.
msg149423 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-12-14 05:25
My guess from the way the docs are written now is that subclassing from Action and over-riding just the __call__ method is the intended way, not just the recommended. If so, Action is a quasi-private class, only exposed so it could be subclassed with one method given. And if so, one could question whether anything more need be documented.

However, in your patch, you write

"+Many actions also override the ``__init__`` method, validating the parameters to the argument definition and raising a ValueError or other Exception on failure."

What 'many actions' are you referring to? Ones you have written, by referring to the code? Ones you think might be written if the doc were added?

In any case, I would prefer input from Stephen before we expose and thereby freeze the __init__ signature.
msg149450 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2011-12-14 13:54
Most of the Action subclasses in argparse override __init__ and they raise ValueErrors when the parameters aren't as expected for that argument. This was my reason for adding that comment. If the basic Actions require this level of validation, wouldn't a custom action also want to supply this type of behavior? I'm sure I've overridden Action.__init__ in some of the subclasses I've created, but I don't remember specifically why.

I agree freezing the __init__ signature is undesirable. It would be preferable if the API called a hook for validating the argument parameters against the action. My intention, however, was to document the existing behavior, not influence changes in the behavior.

Perhaps the recommended API really isn't to subclass Action, but to supply a callable that takes the __init__ args which validates the parameters and returns a callable which takes the __call__ args which should set attributes on the namespace. Perhaps the Action class is only one such implementation of the API. Indeed, that was my understanding of the API as it is currently documented before Terry suggested otherwise.

What do you think about instead of documenting the Action class, we formalize the API, similar to how I described it in the previous paragraph, and then suggest that an Action subclass with an overridden __call__ is the most direct way to implement the Action API?


In reviewing the code again, I note that not just most, but all of the Action subclasses override __init__. I also note that not all of them accept the same parameters. For example, the _StoreConstAction does not accept an nargs parameter.

>>> p = argparse.ArgumentParser()
>>> p.add_argument('--awesome-sauce', action="store_const", const=True, nargs=None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\python\lib\argparse.py", line 1281, in add_argument
    action = action_class(**kwargs)
TypeError: __init__() got an unexpected keyword argument 'nargs'

So it seems that the Action API is slightly different than I described. The first callable should accept all of the parameters passed to add_argument _except_ for the action parameter itself. I think by indicating this in the API description, we avoid the problem of freezing any additional signature.

I'll take another stab at updating the documentation with these findings.
msg149524 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2011-12-15 10:16
Sorry about being out of contact (I'm flying back and forth between the US and the EU every 4-5 weeks right now), and thanks Terry for bringing this to my attention.

Essentially, the API is:

* The argument to action= must be a callable that accepts at least a "dest" and "option_strings" keyword arguments, and returns some object. The keyword arguments passed to this callable will be dest, option_strings, and whatever other keyword arguments were passed to add_argument().

* The object returned by the action= callable should have attributes "dest", "option_strings", "default", "type", "required", "help", etc. defined in essentially the same way as the add_argument() documentation.

* The object returned by the action= callable should itself be callable, and should accept the arguments (self, parser, namespace, values, option_string=None). This method can do whatever it wants, but as you note, the typical approach is to invoke setattr(namespace, self.dest, ...)

Now, all that said, the easiest way of creating a callable that returns an callable where both have the right signatures and the right attributes is to subclass argparse.Action. In fact, doing it any other way is probably crazy. I'm against changing the name of __call__ to invoke because it removes the possibility of defining an action as a function returning another function (which is currently possible), but I'm all for making the documentation strongly recommend subclassing argparse.Action. I would just say something like:

"You may also specify an arbitrary action by passing a subclass of :class:`argparse.Action`, or another callable object that implements the same interface."

I wouldn't bother to go into more detail about "implements the same interface" - all sane people will just subclass Action. ;-)

As to argparse.Action.__init__, hopefully the above bullet points make it clear that you must accept "dest" and "option_strings", but what other keyword arguments you want to accept are up to you. Be sure to call the superclass __init__ to set any attributes that you didn't accept to appropriate default values. A simple example of accepting an additional keyword argument is the _VersionAction, which accepts a "version" keyword argument that the other actions don't accept. The current "example of a custom action" should probably define __init__ to show how this works.
msg223241 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-16 17:43
@Paul what is your opinion of this documentation patch?
msg223273 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2014-07-16 20:54
That's interesting that there's activity on this again. I had forgotten I'd filed it. It's particularly timely, as I just yesterday had this experience again. I looked at the documentation and found it inadequate for me to understand how to implement a custom action, so I went to the source to figure it out. The action I ended up creating was this one:

class SandboxAction(argparse.Action):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, nargs=0, **kwargs)

    def __call__(self, *args, **kwargs):
        cls.use()

Where 'cls' is defined in the same scope where SandboxAction was defined. I'm not sure if my documentation patch would have been sufficient to allow me to write that Action without consulting the source, but this example represents another case where that should be possible.

I haven't made any tweaks to the patch as a result of Steven's comments, so I probably could do that. I don't now when I'll have the time to do that, but I'll keep a tab to remind me to revisit the issue again.
msg223514 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2014-07-20 15:29
I've made another commit to capture Steven's recommendations. I've also merged the work with the latest 2.7 tip, the result of which is the latest diff. I'll commit this in a week or two if there aren't any objections, though I would appreciate a review and especially any suggestions on how I might test the correctness of the syntax.
msg223515 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2014-07-20 15:33
I've tried building the docs, but it appears to rely on a local build of Python as well as subversion, neither of which my system can do currently, so I'm hoping someone has a suitable environment to help test the building of the docs.
msg223523 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-07-20 16:57
I think you want to unlink 9ac347a7f375.diff 

It's from a different project
msg224650 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2014-08-03 18:38
@paul.j3, It wasn't from a different project, it's just that the bug tracker when it does a diff always uses the tip of each branch instead of the most recent common ancestor, so it effectively calculated the the diff of the changes plus the inverse of every change since the effort initiated. But yes, it makes sense to remove it.
msg224651 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-08-03 19:30
New changeset 956c6d33a57d by Jason R. Coombs in branch '2.7':
Issue #13540: Expanded argparse documents to clarify the action API
http://hg.python.org/cpython/rev/956c6d33a57d

New changeset 008a5473f300 by Jason R. Coombs in branch '2.7':
Issue #13540: Removed redundant documentation about Action instance attributes. Updated example and documentation per recommendations by Steven Bethard in msg149524.
http://hg.python.org/cpython/rev/008a5473f300

New changeset 7a627bc9d40e by Jason R. Coombs in branch '2.7':
Issue #13540: Update references to Action class to match syntax used for other classes in this file.
http://hg.python.org/cpython/rev/7a627bc9d40e

New changeset b232e937e668 by Jason R. Coombs in branch '2.7':
Issue #13540: Merge commits
http://hg.python.org/cpython/rev/b232e937e668
msg224652 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-08-03 19:31
New changeset e2c9e0a3ef02 by Jason R. Coombs in branch '3.2':
Issue #13540: Expanded argparse documents to clarify the action API
http://hg.python.org/cpython/rev/e2c9e0a3ef02

New changeset c10a6ca9cb32 by Jason R. Coombs in branch '3.2':
Issue #13540: Removed redundant documentation about Action instance attributes. Updated example and documentation per recommendations by Steven Bethard in msg149524.
http://hg.python.org/cpython/rev/c10a6ca9cb32

New changeset 634f3fe8cbde by Jason R. Coombs in branch '3.2':
Issue #13540: Update references to Action class to match syntax used for other classes in this file.
http://hg.python.org/cpython/rev/634f3fe8cbde

New changeset a36d469f31c1 by Jason R. Coombs in branch '3.3':
Issue #13540: Merge changes from 3.2
http://hg.python.org/cpython/rev/a36d469f31c1

New changeset c689156580ca by Jason R. Coombs in branch '3.4':
Issue #13540: Merge changes from 3.3
http://hg.python.org/cpython/rev/c689156580ca

New changeset a2d01ed713cb by Jason R. Coombs in branch 'default':
Issue #13540: Merge changes from 3.4
http://hg.python.org/cpython/rev/a2d01ed713cb
msg224653 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2014-08-03 19:34
Commits applied to Python 2.7 and 3.2-3.5.
msg225631 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-08-22 01:06
The formatting of the descriptor line for this class is not consistent with the classes for this module.

It lacks 'class'.

It is all bold

In contrast 

    class argparse.ArgumentParser(prog=None, ...

uses bold for only the name.
msg225756 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2014-08-23 18:41
I have no idea why that would be. I used the same syntax for the Action class as ArgumentParser. From the latest tip:

.. class:: ArgumentParser(prog=None, usage=None, description=None, \
                          epilog=None, parents=[], \
                          ...

.. class:: Action(option_strings, dest, nargs=None, const=None, default=None,
                  type=None, choices=None, required=False, help=None,
                  metavar=None)


I don't know what would cause those to render differently. Perhaps you're suggesting a difference elsewhere. In any case, I don't see any action I can take to address your concern.
msg225757 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-08-23 19:17
Maybe the problem is with the latest documentation build, not your patch.
msg225780 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-08-24 01:33
You need line continuation characters '\'


.. class:: Action(option_strings, dest, nargs=None, const=None, default=None, \
                  type=None, choices=None, required=False, help=None, \
                  metavar=None)
msg225788 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-08-24 02:23
New changeset 4a6b71d8575e by Terry Jan Reedy in branch '3.4':
Issue #13540: add missing markup.
http://hg.python.org/cpython/rev/4a6b71d8575e
msg225789 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-08-24 02:25
I don't understand why, but the change worked, and is consistent with similar constructs in the same file. Pushed.

Jason -- security-fix-only versions do not get doc patches. They are released as source only.
msg225794 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-08-24 05:14
While we are talking the Action class, the documentation isn't clear that the 'add_argument' method returns an Action object, specifically one the subclasses.  More than once, when answering Stackoverflow questions I've recommended assigning this object to a variable for use later in the script. 

    a = parser.add_argument(...)
    ...
    print a.dest
    a.help = 'change the help'
    a.required = True
    ...

The doc mentions that 'add_subparsers' 'returns a special action object.'

In sum, I think the documentation needs a sentence or two that describe what add_argument returns.  I'm less sure whether it should include an example of using that return.
History
Date User Action Args
2014-08-24 05:14:28paul.j3setmessages: + msg225794
2014-08-24 02:25:03terry.reedysetmessages: + msg225789
2014-08-24 02:23:02python-devsetmessages: + msg225788
2014-08-24 01:33:01paul.j3setmessages: + msg225780
2014-08-23 19:17:19paul.j3setmessages: + msg225757
2014-08-23 18:41:55jaracosetmessages: + msg225756
2014-08-22 01:06:11paul.j3setmessages: + msg225631
2014-08-03 19:34:22jaracosetstatus: open -> closed
resolution: fixed
messages: + msg224653
2014-08-03 19:31:48python-devsetmessages: + msg224652
2014-08-03 19:30:55python-devsetnosy: + python-dev
messages: + msg224651
2014-08-03 18:38:31jaracosetfiles: - 9ac347a7f375.diff
2014-08-03 18:38:11jaracosetmessages: + msg224650
2014-07-20 16:57:58paul.j3setmessages: + msg223523
2014-07-20 15:33:41jaracosetmessages: + msg223515
2014-07-20 15:29:00jaracosetmessages: + msg223514
2014-07-20 15:22:27jaracosetfiles: + b8c09b766e20.diff
2014-07-20 15:00:21jaracosetfiles: + 9ac347a7f375.diff
2014-07-16 20:54:04jaracosetmessages: + msg223273
2014-07-16 17:43:56BreamoreBoysetnosy: + paul.j3, BreamoreBoy

messages: + msg223241
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2012-03-18 21:58:16tshepangsetnosy: + tshepang
2011-12-15 10:16:38bethardsetmessages: + msg149524
2011-12-14 13:54:30jaracosetmessages: + msg149450
2011-12-14 05:25:27terry.reedysetmessages: + msg149423
2011-12-14 04:46:35jaracosetfiles: + 956c6d33a57d.diff
keywords: + patch
2011-12-14 04:44:38jaracosetmessages: + msg149422
2011-12-14 04:39:35jaracosethgrepos: + hgrepo95
2011-12-13 15:08:30jaracosetmessages: + msg149391
2011-12-12 22:58:23terry.reedysetnosy: + terry.reedy
messages: + msg149363
2011-12-10 16:21:43eric.araujosetnosy: + bethard, eric.araujo
2011-12-06 14:57:00jaracosettitle: Document the Action API -> Document the Action API in argparse
2011-12-06 14:56:34jaracocreate