Argparser does not display closing parentheses in nested mutex groups

[christofsteel]

BPO	29553
Nosy	@rhettinger, @berkerpeksag, @andrewnester, @christofsteel, @mental32
PRs	bpo-29553: Fixed ArgumentParses format_usage for mutually exclusive groups #117 Alternarive for bpo-29553 - Fixed ArgumentParses format_usage for mutually exclusive groups #120 bpo-29553: Fixed ArgumentParses format_usage for mutually exclusive groups #14976 [3.7] bpo-29553: Fix ArgumentParser.format_usage() for mutually exclusive groups (GH-14976) #15494 [3.8] bpo-29553: Fix ArgumentParser.format_usage() for mutually exclu… #15624

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/rhettinger'
closed_at = <Date 2019-08-31.00:15:12.302>
created_at = <Date 2017-02-14.14:59:53.702>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'Argparser does not display closing parentheses in nested mutex groups'
updated_at = <Date 2019-08-31.00:15:12.301>
user = 'https://github.com/christofsteel'

bugs.python.org fields:

activity = <Date 2019-08-31.00:15:12.301>
actor = 'berker.peksag'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2019-08-31.00:15:12.302>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2017-02-14.14:59:53.702>
creator = 'christofsteel'
dependencies = []
files = []
hgrepos = []
issue_num = 29553
keywords = ['patch']
message_count = 26.0
messages = ['287776', '287840', '287851', '287852', '287856', '287937', '287941', '288138', '288382', '288418', '288607', '288609', '288613', '290084', '290138', '291038', '293558', '339217', '350469', '350473', '350849', '350888', '350891', '350894', '350895', '350901']
nosy_count = 7.0
nosy_names = ['rhettinger', 'bethard', 'berker.peksag', 'paul.j3', 'andrewnester', 'christofsteel', 'mental']
pr_nums = ['117', '120', '14976', '15494', '15624']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29553'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[christofsteel]

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]]]

[andrewnester]

I've just added PR fixing this.

[christofsteel]

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.

[briancurtin]

Dropping the inner brackets sounds like a better move to me.

[andrewnester]

Ive just added alternative PR that drops inner brackets. So we've got options to choose!

[andrewnester]

any updates on this?

[briancurtin]

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.

[berkerpeksag]

FWIW, I also prefer PR 120 over PR 117. However, if Steven prefers PR 120 we probably should merge it only in master.

[paulj3]

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.

[paulj3]

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

[paulj3]

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.

[paulj3]

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.

[andrewnester]

JFYI, from my perspective as a developer PR 120 is more preferred one.

[andrewnester]

any updates?

[paulj3]

How would you rank this issue relative to other argparse ones? Currently I'm following 123 open issues.

[andrewnester]

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

[andrewnester]

so any feedback on this?

[mental32]

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).

[berkerpeksag]

New changeset da27d9b by Berker Peksag (Flavian Hautbois) in branch 'master':
bpo-29553: Fix ArgumentParser.format_usage() for mutually exclusive groups (GH-14976)
da27d9b

[berkerpeksag]

New changeset 31ea447 by Berker Peksag (Miss Islington (bot)) in branch '3.7':
bpo-29553: Fix ArgumentParser.format_usage() for mutually exclusive groups (GH-14976)
31ea447

[rhettinger]

Is this resolved or this there still more to do?

[berkerpeksag]

da27d9b needs to be manually backported to 3.8.

[rhettinger]

Okay, I'll do the backport.

[rhettinger]

New changeset bd8ca9a 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)
bd8ca9a

[rhettinger]

The backport is done.

Can we close this now (and PR 120 which is still shown as open)?

[berkerpeksag]

Yes, we can. Thank you for the backport, Raymond :)