classification
Title: Argparser does not display closing parentheses in nested mutex groups
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: andrewnester, berker.peksag, bethard, christofsteel, mental, paul.j3, rhettinger
Priority: normal Keywords: patch

Created on 2017-02-14 14:59 by christofsteel, last changed 2019-08-31 00:15 by berker.peksag. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 117 closed andrewnester, 2017-02-15 10:31
PR 120 closed andrewnester, 2017-02-15 14:21
PR 14976 merged flavianhautbois, 2019-07-27 08:20
PR 15494 merged miss-islington, 2019-08-25 19:07
PR 15624 merged rhettinger, 2019-08-30 21:48
Messages (26)
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) * (Python triager) 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) * (Python triager) 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) * (Python triager) 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) * (Python triager) 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.
msg290084 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-03-24 11:28
any updates?
msg290138 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2017-03-24 20:26
How would you rank this issue relative to other `argparse` ones?  Currently I'm following 123 open issues.
msg291038 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-04-02 15:56
From my perspective current behaviour is a bit frustrate that's why it would be nice to have this issue fixed, but I would say it's critic one.
At the same time it doesn't introduce any BC breaking changes and kind safe
msg293558 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-05-12 16:24
so any feedback on this?
msg339217 - (view) Author: (mental) * Date: 2019-03-30 18:29
Can this issue be closed?

It's been inactive for a while and all it needs is a contributor to merge and close.

Seems to me it's been resolved with PRs 117 and 120, the difference between them being 120 drops the inner brackets for the format usage (imo 120 should be merged and 117 should be closed).
msg350469 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-08-25 19:06
New changeset da27d9b9dc44913ffee8f28d9638985eaaa03755 by Berker Peksag (Flavian Hautbois) in branch 'master':
bpo-29553: Fix ArgumentParser.format_usage() for mutually exclusive groups (GH-14976)
https://github.com/python/cpython/commit/da27d9b9dc44913ffee8f28d9638985eaaa03755
msg350473 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-08-25 21:30
New changeset 31ea447ffe591736af1d7a3178c0f7ca3eb50d70 by Berker Peksag (Miss Islington (bot)) in branch '3.7':
bpo-29553: Fix ArgumentParser.format_usage() for mutually exclusive groups (GH-14976)
https://github.com/python/cpython/commit/31ea447ffe591736af1d7a3178c0f7ca3eb50d70
msg350849 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-08-30 04:09
Is this resolved or this there still more to do?
msg350888 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-08-30 21:02
da27d9b9dc44913ffee8f28d9638985eaaa03755 needs to be manually backported to 3.8.
msg350891 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-08-30 21:34
Okay, I'll do the backport.
msg350894 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-08-30 22:25
New changeset bd8ca9aaccef0569c4b5c2e6dad0feb869ffd6c5 by Raymond Hettinger in branch '3.8':
[3.8] bpo-29553: Fix ArgumentParser.format_usage() for mutually exclusive groups (GH-14976) (GH-15494) (GH-15624)
https://github.com/python/cpython/commit/bd8ca9aaccef0569c4b5c2e6dad0feb869ffd6c5
msg350895 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-08-30 22:27
The backport is done.

Can we close this now (and PR 120 which is still shown as open)?
msg350901 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-08-31 00:15
Yes, we can. Thank you for the backport, Raymond :)
History
Date User Action Args
2019-08-31 00:15:12berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg350901

stage: patch review -> resolved
2019-08-30 22:27:30rhettingersetmessages: + msg350895
2019-08-30 22:25:44rhettingersetmessages: + msg350894
2019-08-30 21:48:11rhettingersetstage: backport needed -> patch review
pull_requests: + pull_request15292
2019-08-30 21:34:51rhettingersetmessages: + msg350891
2019-08-30 21:02:00berker.peksagsetstage: patch review -> backport needed
messages: + msg350888
versions: + Python 3.8, - Python 2.7, Python 3.5
2019-08-30 04:09:42rhettingersetmessages: + msg350849
2019-08-30 03:30:36rhettingersetassignee: bethard -> rhettinger

nosy: + rhettinger
2019-08-25 21:30:57berker.peksagsetmessages: + msg350473
2019-08-25 19:07:00miss-islingtonsetpull_requests: + pull_request15182
2019-08-25 19:06:49berker.peksagsetmessages: + msg350469
2019-07-27 08:20:31flavianhautboissetkeywords: + patch
pull_requests: + pull_request14745
2019-03-30 18:29:48mentalsetnosy: + mental
messages: + msg339217
2017-05-12 16:24:44andrewnestersetmessages: + msg293558
2017-04-02 15:56:06andrewnestersetmessages: + msg291038
2017-03-24 20:59:18brian.curtinsetnosy: - brian.curtin
2017-03-24 20:26:50paul.j3setmessages: + msg290138
2017-03-24 11:28:27andrewnestersetmessages: + msg290084
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