classification
Title: Argparser does not display closing parentheses in nested mutex groups
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: bethard Nosy List: andrewnester, berker.peksag, bethard, brian.curtin, christofsteel, paul.j3
Priority: normal Keywords:

Created on 2017-02-14 14:59 by christofsteel, last changed 2017-02-26 21:06 by andrewnester.

Pull Requests
URL Status Linked Edit
PR 117 open andrewnester, 2017-02-15 10:31
PR 120 open andrewnester, 2017-02-15 14:21
Messages (13)
msg287776 - (view) Author: Christoph Stahl (christofsteel) Date: 2017-02-14 14:59
When creating nested mutually exclusive groups, all closing brackets except one are omitted.

Example:
parser = ArgumentParser()
group = parser.add_mutually_exclusive_group()
group.add_argument('-a')
group.add_argument('-b')
group2 = group.add_mutually_exclusive_group()
group2.add_argument('-c')
group2.add_argument('-d')
group3 = group2.add_mutually_exclusive_group()
group3.add_argument('-e')
group3.add_argument('-f')

prints a usage line of:
usage: test.py [-h] [-a A | -b B | [-c C | -d D | [-e E | -f F]

it should print something like:
usage: test.py [-h] [-a A | -b B | [-c C | -d D | [-e E | -f F]]]
msg287840 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-02-15 10:32
I've just added PR fixing this.
msg287851 - (view) Author: Christoph Stahl (christofsteel) Date: 2017-02-15 13:20
Hi,

I thought a bit about the problem and came up with the following:

The | in the usage is de facto an XOR operator. Exactly one of the options can be used. The XOR operator has the associative property, meaning:

  (A XOR B) XOR C = A XOR (B XOR C)

So translated to the | this means:
  
  [[ -a | -b ] | -c ] = [ -a | [ -b | -c ]]

usually one writes:

  [ -a | -b | -c ]

So I propose dropping the inner brackets altogether.
msg287852 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2017-02-15 13:29
Dropping the inner brackets sounds like a better move to me.
msg287856 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-02-15 14:22
Ive just added alternative PR that drops inner brackets. So we've got options to choose!
msg287937 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-02-16 13:31
any updates on this?
msg287941 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2017-02-16 13:39
PR 120 looks fine to me, but Steven Bethard is the maintainer of argparse so he's better suited to say for sure if exclusive groups are ok how they are in 120 or if 117 is actually the way forward.
msg288138 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-02-19 14:24
FWIW, I also prefer PR 120 over PR 117. However, if Steven prefers PR 120 we probably should merge it only in master.
msg288382 - (view) Author: paul j3 (paul.j3) * Date: 2017-02-22 20:24
http://bugs.python.org/issue22047 "argparse improperly prints mutually exclusive options when they are in a group"

is similar.

-----

There are two issues:

- the nesting of mutually exclusive groups

- the formatting of the usage in such cases.

Both have come up in one way or other in other bug reports.

Yes, it is possible to add a group to an existing group.  But from a testing standpoint the effect is the same as if you put all actions into one super group. 

More often people try to nest ArgumentGroups in MutuallyExclusiveGroups thinking that would give some sort of 'any' or 'and' logic within the 'xor' logic.  I have explored that in 

http://bugs.python.org/issue11588.  

Defining nestable groups is relatively easy.  Writing good usage is much harder.
 
The usage formatter is brittle.  It creates a big string, strips out 'excess' characters (the problem here), and then splits and reassembles the string (leading to assertion errors if the metavars contain funny characters).

In 

http://bugs.python.org/issue11874  

I submitted a patch that substantially rewrites the usage formatter.  The idea was to keep the action strings separate until the last moment.  While I haven't tested it with the current problem, I did use it in my nested-groups coding.

While I'm not opposed to patching the usage formatting in bits and pieces, we should do so while fully aware of the big picture.  Little patches tend to make brittle code even more brittle.
msg288418 - (view) Author: paul j3 (paul.j3) * Date: 2017-02-23 07:03
I played around with the patch 117.  I was wrong in thinking this was another case of excess brackets being wrongly purged.  The fix works by adding ending ] that were missing the original.  And it does add a symmetry to the code.  

But it is easy to construct a set of Actions that creates further errors.  For the 'inserts' to work correctly the mutually exclusive Actions have to be defined together.  So all the actions of the nested group have to be defined after the non nest actions of the parent group.  An optional positional can also cause problems.

This whole _format_actions_usage method was not written with nested groups in mind.  While this patch fixes one or two cases, it isn't robust.

I also don't like the idea of enshrining group nesting in the test_argparse.py file.  That fact that it works as well as it does is an accident, a feature, not an intentional behavior.

I haven't tested PR120 yet.  Dropping the inner brackets gives a cleaner display.  The nested brackets of 117 are hard to read, even if they are correct.  

Note that in http://bugs.python.org/issue22047 I proposed blocking nested groups, by raising an error.

The reason why it is possible to add either kind of group to a _MutuallyExclusiveGroup is because it inherits the add methods from _ActionsContainer.  Normally those group add methods are used by ArgumentParser which also inherits from _ActionsContainer.

It is possible to add a MutuallyExclusiveGroup to an ArgumentGroup, and that is somewhat useful.  It doesn't change the usage, but does let you group that set of Actions in the help lines.  But this nesting is not fully developed, as hinted at by a commeent in the method that copies 'parents':

def _add_container_actions
        # add container's mutually exclusive groups
        # NOTE: if add_mutually_exclusive_group ever gains title= and
        # description= then this code will need to be expanded as above
msg288607 - (view) Author: paul j3 (paul.j3) * Date: 2017-02-26 18:15
The PR117 patch adds an apparent symmetry.  There's a if/else for 'start', so shouldn't there also be one for 'end'?

    if start in inserts:
        inserts[start] += ' ['
    else:
        inserts[start] = '['

But why the '+=' case?  It's needed for cases like this:

    usage: prog [-h] [-A | -B] [-C | -D]

It ensures that the insert between B and C is '] [', as opposed to just '['. 

Without any groups usage for the 4 Actions is

    usage: prog [-h] [-A] [-B] [-C] [-D]

Adding the first group adds a '[','|', and ']' (the inner [] will be deleted later).  The second group also wants to add '[','|',']', but its '[' has to be concatenated to the existing ']', rather than replace it.

-----

To understand what's happening when we nest groups, we have to look at the resulting groups.  

Usage is created with

    formatter.add_usage(self.usage, self._actions,
                            self._mutually_exclusive_groups)

  
In christofsteel's original example, the parser._mutually_exclusive_groups is a len 3 list.  Each group has a list of '_group_actions'.  

    In [13]: [len(g._group_actions) for g in parser._mutually_exclusive_groups]
    Out[13]: [6, 4, 2]

The first, outermost group, contains all the defined Actions, including the ones defined in the nested groups.  It doesn't contain 2 actions and a group. The link between child and parent group is one way.  The child knows its 'container', but the parent has not information about 'children'. 

Usage using just the 1st group produces:

    usage: ipython3 [-h] [-a A | -b B | -c C | -d D | -e E | -f F]

With 2 groups:

    usage: ipython3 [-h] [-a A | -b B | [-c C | -d D | -e E | -f F]

The second group has added it's '[ | | | ]' on top of the first group's inserts.  The '[' was appended, the others over write.

That's more apparent if I change the 2nd group to be 'required':

    usage: ipython3 [-h] [-a A | -b B | (-c C | -d D | -e E | -f F)

The final ']' (from the 1st group) has been replaced with a ')'.

The patch ensures that the new ']' is appended to the existing ']'. But if the 2nd group is required, the patch will produce:

     | -f F])

not  

     | -f F)]

as would be expected if the groups were really nested.

In sum, patching brittle code to do something it wasn't designed to do in the first place isn't the solution.

Disabling nesting as recommended in http://bugs.python.org/issue22047, is, I think a better solution.

---

An old (2011) issue tries to put an action in 2 or more groups:

http://bugs.python.org/issue10984 'argparse add_mutually_exclusive_group should accept existing arguments to register conflicts'

Adding an existing action to a new group is relatively easy.  But usage display runs into the same issues.  I had to recommend a total rewrite that ditches the 'inserts' approach entirely.
msg288609 - (view) Author: paul j3 (paul.j3) * Date: 2017-02-26 19:15
I should probably give PR 120 more credit.  By checking the group's container it in effect eliminates this overlapping action problem.  Nested groups aren't used in the usage, just the union xor.

Maybe the question is, which is better for the user?  To tell them up front that nesting is not allowed, or to display the union group without further comment?  Does allowing nesting on the input give them a false sense of control?

More often on StackOverFlow users try to nest ArgumentGroups in a mutually exclusive one, thinking that this will give some sort of 'any-of' logic.
msg288613 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-02-26 21:06
JFYI, from my perspective as a developer PR 120 is more preferred one.
History
Date User Action Args
2017-02-26 21:06:33andrewnestersetmessages: + msg288613
2017-02-26 19:15:41paul.j3setmessages: + msg288609
2017-02-26 18:15:36paul.j3setmessages: + msg288607
2017-02-23 07:03:01paul.j3setmessages: + msg288418
2017-02-22 20:24:41paul.j3setmessages: + msg288382
2017-02-22 13:54:25xiang.zhangsetnosy: + paul.j3
2017-02-19 14:24:50berker.peksagsetversions: + Python 3.5, Python 3.7
nosy: + berker.peksag

messages: + msg288138

stage: patch review
2017-02-16 13:39:20brian.curtinsetassignee: bethard
messages: + msg287941
2017-02-16 13:31:43andrewnestersetmessages: + msg287937
2017-02-15 14:22:00andrewnestersetmessages: + msg287856
2017-02-15 14:21:23andrewnestersetpull_requests: + pull_request80
2017-02-15 13:29:31brian.curtinsetnosy: + brian.curtin, bethard
messages: + msg287852
2017-02-15 13:20:36christofsteelsetmessages: + msg287851
2017-02-15 10:32:47andrewnestersetnosy: + andrewnester
messages: + msg287840
2017-02-15 10:31:50andrewnestersetpull_requests: + pull_request76
2017-02-14 16:25:00christofsteelsettype: behavior
2017-02-14 14:59:53christofsteelcreate