[issue22401] argparse: 'resolve' conflict handler damages the actions of the parent parser
New submission from paul j3:
When there's a conflict involving an argument that was added via 'parents', and the conflict handler is 'resolve', the action in the parent parser may be damaged, rendering that parent unsuitable for further use.
In this example, 2 parents have the same '--config' argument:
parent1 = argparse.ArgumentParser(add_help=False)
parent1.add_argument('--config')
parent2 = argparse.ArgumentParser(add_help=False)
parent2.add_argument('--config')
parser = argparse.ArgumentParser(parents=[parent1,parent2],
conflict_handler='resolve')
The actions of the 3 parsers are (from the ._actions list):
(id, dest, option_strings)
parent1: [(3077384012L, 'config', [])] # empty option_strings
parent2: [(3076863628L, 'config', ['--config'])]
parser: [(3076864428L, 'help', ['-h', '--help']),
(3076863628L, 'config', ['--config'])] # same id
The 'config' Action from 'parent1' is first copied to 'parser' by reference (this is important). When 'config' from 'parent2' is copied, there's a conflict. '_handle_conflict_resolve()' attempts to remove the first Action, so it can add the second. But in the process it ends up deleting the 'option_strings' values from the original action.
So now 'parent1' has an action in its 'optionals' argument group with an empty option_strings list. It would display as an 'optionals' but parse as a 'positionals'. 'parent1' can no longer be safely used as a parent for another (sub)parser, nor used as a parser itself.
The same sort of thing would happen, if, as suggested in the documentation:
"Sometimes (e.g. when using parents_) it may be useful to simply
override any older arguments with the same option string."
In test_argparse.py, 'resolve' is only tested once, with a simple case of two 'add_argument' statements. The 'parents' class tests a couple of cases of conflicting actions (for positionals and optionals), but does nothing with the 'resolve' handler.
------------------------------
Possible fixes:
- change the documentation to warn against reusing such a parent parser
- test the 'resolve' conflict handler more thoroughly
- rewrite this conflict handler so it does not modify the action in the parent
- possibly change the 'parents' mechanism so it does a deep copy of actions.
References:
http://stackoverflow.com/questions/25818651/argparse-conflict-resolver-for-o...
http://bugs.python.org/issue15271
argparse: repeatedly specifying the same argument ignores the previous ones
http://bugs.python.org/issue19462
Add remove_argument() method to argparse.ArgumentParser
http://bugs.python.org/issue15428
add "Name Collision" section to argparse docs
----------
assignee: docs@python
components: Documentation, Library (Lib), Tests
messages: 226862
nosy: docs@python, paul.j3
priority: normal
severity: normal
status: open
title: argparse: 'resolve' conflict handler damages the actions of the parent parser
type: behavior
versions: Python 3.5
_______________________________________
Python tracker
paul j3 added the comment:
'sample3.py' contains a '_handle_conflict_parent' function.
With the help of a change in '_add_container_actions' it determines whether a conflicting action occurs in multiple parsers (and argument_groups). If it does, it deletes it from the correct group, without altering either the action, or other parsers.
If the action occurs in only 1 group, then the 'resolve' method is used.
The testing script mixes parents, subparsers, and various conflicts.
----------
Added file: http://bugs.python.org/file36635/sample3.py
_______________________________________
Python tracker
Changes by paul j3
Changes by paul j3
Changes by paul j3
Changes by paul j3
Changes by paul j3
Changes by paul j3
Changes by paul j3
paul j3 added the comment:
A simpler solution is to make a copy of each Action when importing a parent parser. The current practice is to just copy references. With a copy, an Action will belong to only one group and parser, and the 'resolve' handler will operate without problems.
In the attached patch, I added a `.copy` method to Action. I believe '.option_strings' is the only attribute that needs special handling, since it is a list, and 'resolve' operates on it. The other attributes are strings, or objects that the user defines (e.g. 'default', 'choices').
The other change is in '_add_container_actions', the method which imports parents. Here I make the copy action contingent on a 'action_copy' attribute of the conflict handler object (a function). I also define this attribute for the 'resolve' handler. I've made this copy contingent just to be safe w/r to backward compatibility, even though, I can't think of a reason for preferring the existing copy by reference.
In another Stackoverflow question, a poster wanted to use the same parent for 2 subparsers, but give the 2 actions different defaults. This copy approach solves that issue.
This patch needs testing and documentation changes.
----------
keywords: +patch
Added file: http://bugs.python.org/file36773/patch_1.diff
_______________________________________
Python tracker
Jared Deckard added the comment:
This behavior is preventing me from using more than one parent parser.
My use case is a convenience subcommand that performs two existing subcommands, therefore logically its subparser is required to support the arguments of both subparsers.
The only conflict is the "help" argument, which is annoying, but setting the conflict handler to "resolve" on the child subparser should just ignore the first parent's "help" argument in favor of the second parent.
Instead the "resolve" conflict handler breaks the first parent and causes it to output the help info no matter what arguments are given to it.
I am forced to only include one parent and manually duplicate the arguments for any additional parents.
----------
components: -Documentation, Tests
nosy: +deckar01
versions: +Python 2.7 -Python 3.5
_______________________________________
Python tracker
paul j3 added the comment:
Is this the kind of scenario that you have problems with?
parser = argparse.ArgumentParser()
sp = parser.add_subparsers(dest='cmd')
p1 = sp.add_parser('cmd1')
p1.add_argument('-f')
p2 = sp.add_parser('cmd2')
p2.add_argument('-b')
p3 = sp.add_parser('cmd3', parents=[p1,p2],add_help=False, conflict_handler='resolve')
p3.add_argument('-c')
The problem is, apparently, that 'resolve' removes the '-h' option_string from the 1st subparser (but does not delete the whole action). A kludgy fix is to add the '-h' back in (after p3 is created):
p1._actions[0].option_strings=['-h']
'p1._actions' is the list of actions (arguments) for subparser p1. Usually help is the first action (if isn't in p3 because of the parents resolve action).
I may work out a custom conflict handler function, but for now this appears to solve the issue. I don't know if the patch I proposed solves your issue or not.
----------
_______________________________________
Python tracker
Jared Deckard added the comment: That is correct. (Thank you for whipping up a repro script from my description). I appreciate the work around, but it is nearly as verbose as manually duplicating the parameters on the child and I would have to type up a large comment block to educate future developers on the hack. Is there some documentation I am missing or is the "resolve" conflict handler broken?
The parents= argument takes a list of ArgumentParser objects, collects all the positional and optional actions from them, and adds these actions to the ArgumentParser object being constructed. https://docs.python.org/3.6/library/argparse.html#parents
It would be nice if the "error" conflict handler ignored conflicts when the actions are identical.
It should be expected that the "resolve" conflict handler would not modify the parents. It is exhibiting undocumented and undesirable side effect, which indicates to me that it is a bug not a missing feature.
As for the patch, I'm not sure why you would ever want "action_copy" to be False. It seems like creating copies to insulate the originals ignores the underlying issue that these parents are being mutated when the operation implies a union of properties with no side effects.
I don't think the fix for this should be behind an optional flag, I think the fix is to update "resolve" to take into consideration multiple parents like the custom conflict handler in your sample.
----------
_______________________________________
Python tracker
Jared Deckard added the comment:
Adding back components and version data I unintentionally removed in http://bugs.python.org/msg262314
----------
components: +Documentation, Tests
versions: +Python 3.5
_______________________________________
Python tracker
paul j3 added the comment:
Neither 'parents' or 'resolve' are documented in any detail. Most of what I know comes from reading the code.
'parents' are, I think, most useful when importing a parser from some other module. It lets you add arguments to your own parser without digging into the other module's code. And 'resolve' lets you overwrite arguments.
Copying arguments by reference is the easiest thing to do in Python, and apparently no one asked, 'what if the user wants to use the parent independently?'
Note I had to add a 'copy' method, because nothing else in 'argparse' needs it. And no one has written code to check whether two Actions are the same (other than the obvious one of checking object ids). Again, no need.
Copy and paste and utility functions are great ways of defining multiple arguments when mechanisms like 'parents' prove to be too clumsy. Look at `test_argparse.py` for examples of utility code that streamlines parser definition. Each test case requires a new parser (or more).
----------
_______________________________________
Python tracker
Change by Kuang-che Wu
participants (3)
-
Jared Deckard -
Kuang-che Wu
-
paul j3