[issue13540] Document the Action API
New submission from Jason R. Coombs <jaraco@jaraco.com>: 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. ---------- assignee: docs@python components: Documentation messages: 148921 nosy: docs@python, jason.coombs priority: low severity: normal status: open title: Document the Action API type: behavior versions: Python 2.7, Python 3.2, Python 3.3 _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Jason R. Coombs <jaraco@jaraco.com>: ---------- title: Document the Action API -> Document the Action API in argparse _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Éric Araujo <merwok@netwok.org>: ---------- nosy: +bethard, eric.araujo _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: 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. ---------- nosy: +terry.reedy _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs <jaraco@jaraco.com> added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Jason R. Coombs <jaraco@jaraco.com>: ---------- hgrepos: +95 _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs <jaraco@jaraco.com> added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Jason R. Coombs <jaraco@jaraco.com>: ---------- keywords: +patch Added file: http://bugs.python.org/file23949/956c6d33a57d.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs <jaraco@jaraco.com> added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Steven Bethard <steven.bethard@gmail.com> added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Tshepang Lekhonkhobe <tshepang@gmail.com>: ---------- nosy: +tshepang _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Mark Lawrence added the comment: @Paul what is your opinion of this documentation patch? ---------- nosy: +BreamoreBoy, paul.j3 versions: +Python 3.4, Python 3.5 -Python 3.2, Python 3.3 _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Jason R. Coombs <jaraco@jaraco.com>: Added file: http://bugs.python.org/file36001/9ac347a7f375.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Jason R. Coombs <jaraco@jaraco.com>: Added file: http://bugs.python.org/file36003/b8c09b766e20.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
paul j3 added the comment: I think you want to unlink 9ac347a7f375.diff It's from a different project ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs added the comment: @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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Changes by Jason R. Coombs <jaraco@jaraco.com>: Removed file: http://bugs.python.org/file36001/9ac347a7f375.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Roundup Robot added the comment: 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 ---------- nosy: +python-dev _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Roundup Robot added the comment: 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 ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs added the comment: Commits applied to Python 2.7 and 3.2-3.5. ---------- resolution: -> fixed status: open -> closed _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
paul j3 added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Jason R. Coombs added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
paul j3 added the comment: Maybe the problem is with the latest documentation build, not your patch. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
paul j3 added the comment: 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) ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Roundup Robot added the comment: New changeset 4a6b71d8575e by Terry Jan Reedy in branch '3.4': Issue #13540: add missing markup. http://hg.python.org/cpython/rev/4a6b71d8575e ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
Terry J. Reedy added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
paul j3 added the comment: 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13540> _______________________________________
participants (8)
-
Jason R. Coombs -
Mark Lawrence
-
paul j3
-
Roundup Robot
-
Steven Bethard
-
Terry J. Reedy
-
Tshepang Lekhonkhobe
-
Éric Araujo