Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Argparser does not display closing parentheses in nested mutex groups #73739

Closed
christofsteel mannequin opened this issue Feb 14, 2017 · 26 comments
Closed

Argparser does not display closing parentheses in nested mutex groups #73739

christofsteel mannequin opened this issue Feb 14, 2017 · 26 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@christofsteel
Copy link
Mannequin

christofsteel mannequin commented Feb 14, 2017

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
    Copy link
    Mannequin Author

    christofsteel mannequin commented Feb 14, 2017

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

    @christofsteel christofsteel mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 14, 2017
    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 15, 2017

    I've just added PR fixing this.

    @christofsteel
    Copy link
    Mannequin Author

    christofsteel mannequin commented Feb 15, 2017

    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
    Copy link
    Member

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

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 15, 2017

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

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 16, 2017

    any updates on this?

    @briancurtin
    Copy link
    Member

    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
    Copy link
    Member

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

    @berkerpeksag berkerpeksag added the 3.7 (EOL) end of life label Feb 19, 2017
    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Feb 22, 2017

    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
    Copy link
    Mannequin

    paulj3 mannequin commented Feb 23, 2017

    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
    Copy link
    Mannequin

    paulj3 mannequin commented Feb 26, 2017

    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
    Copy link
    Mannequin

    paulj3 mannequin commented Feb 26, 2017

    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
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 26, 2017

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

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Mar 24, 2017

    any updates?

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Mar 24, 2017

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

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Apr 2, 2017

    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
    Copy link
    Mannequin

    andrewnester mannequin commented May 12, 2017

    so any feedback on this?

    @mental32
    Copy link
    Mannequin

    mental32 mannequin commented Mar 30, 2019

    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
    Copy link
    Member

    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
    Copy link
    Member

    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 rhettinger assigned rhettinger and unassigned bethard Aug 30, 2019
    @rhettinger
    Copy link
    Contributor

    Is this resolved or this there still more to do?

    @berkerpeksag
    Copy link
    Member

    da27d9b needs to be manually backported to 3.8.

    @berkerpeksag berkerpeksag added the 3.8 only security fixes label Aug 30, 2019
    @rhettinger
    Copy link
    Contributor

    Okay, I'll do the backport.

    @rhettinger
    Copy link
    Contributor

    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
    Copy link
    Contributor

    The backport is done.

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

    @berkerpeksag
    Copy link
    Member

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants