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

argparse: nargs could accept range of options count #55563

Closed
WM mannequin opened this issue Feb 28, 2011 · 22 comments
Closed

argparse: nargs could accept range of options count #55563

WM mannequin opened this issue Feb 28, 2011 · 22 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@WM
Copy link
Mannequin

WM mannequin commented Feb 28, 2011

BPO 11354
Nosy @rhettinger, @brandtbucher, @shihai1991, @alclarks, @uniqueg
Files
  • argparse-nargs.patch: patch
  • test.py
  • issue11354.patch: patch (lib & tests)
  • prelimary.patch
  • nargsrange.patch
  • 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 = None
    closed_at = <Date 2019-11-15.17:20:18.025>
    created_at = <Date 2011-02-28.16:51:33.255>
    labels = ['type-feature', 'library']
    title = 'argparse: nargs could accept range of options count'
    updated_at = <Date 2021-04-25.07:05:49.360>
    user = 'https://bugs.python.org/wm'

    bugs.python.org fields:

    activity = <Date 2021-04-25.07:05:49.360>
    actor = 'paul.j3'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-11-15.17:20:18.025>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2011-02-28.16:51:33.255>
    creator = 'wm'
    dependencies = []
    files = ['20947', '20981', '21617', '30181', '30187']
    hgrepos = []
    issue_num = 11354
    keywords = ['patch']
    message_count = 22.0
    messages = ['129714', '129715', '129755', '129917', '132240', '133537', '162475', '166171', '188743', '188757', '188795', '204775', '355901', '355902', '355904', '355959', '356558', '356587', '356687', '373435', '389162', '391843']
    nosy_count = 11.0
    nosy_names = ['rhettinger', 'bethard', 'Danh', 'wm', 'paul.j3', 'atfrase', 'brandtbucher', 'shihai1991', 'alclarks', 'calestyo', 'uniqueg']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue11354'
    versions = ['Python 3.4']

    @WM
    Copy link
    Mannequin Author

    WM mannequin commented Feb 28, 2011

    Hi, sometimes it is needed to grab variable, but limited, number of options.
    For example --point could accept 2, 3 or 4 values: (x,y) or (x,y,z) or
    (x,y,z,w). Current version of argparse requires postprocessing:

    	parser.add_argument('--point', action='append', default=[])
    	nmps = parser.parse_args()
    	if not (2 <= len(nmsp.point) <= 4):
    		raise argparse.ArgumentTypeError("--point expects 2, 3 or 4 values")

    I propose to allow pass range of options count to nargs, including
    lower/upper bound. For example:

    parser.add_argument('--point', nargs=(2,4) )	# from 2 to 4
    parser.add_argument('--foo', nargs=(9, None) )	# at least 9
    parser.add_argument('--bar', nargs=(None, 7) )	# at most 7
    nmsp = parser.parse_args()
    

    I've attached tests and patch made against Python3.2 lib from Debian.

    w.

    @WM WM mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Feb 28, 2011
    @WM
    Copy link
    Mannequin Author

    WM mannequin commented Feb 28, 2011

    tests

    @Danh
    Copy link
    Mannequin

    Danh mannequin commented Mar 1, 2011

    Hi Wojciech,

    in your tests, at

        def test_add_argument10(self):
            "nargs = (0, 1) => optimized to '?'"
            opt = self.add_argument(1, None)
            self.assertEqual(opt.nargs, argparse.ONE_OR_MORE)

    you should change "argparse.ONE_OR_MORE" to "argparse.OPTIONAL".

    @WM
    Copy link
    Mannequin Author

    WM mannequin commented Mar 2, 2011

    Daniel, thanks for note, fixed.

    @bethard
    Copy link
    Mannequin

    bethard mannequin commented Mar 26, 2011

    Thanks for the patch. The idea and the approach of the patch look fine. But the patch needs to be against the Python repository:

    http://docs.python.org/devguide/patch.html#creating

    For the tests, you should integrate your test.py into Lib/test/test_argparse.py.

    @WM
    Copy link
    Mannequin Author

    WM mannequin commented Apr 11, 2011

    Steven, thank you for links, I prepared patch against trunk.
    All tests passed.

    @atfrase
    Copy link
    Mannequin

    atfrase mannequin commented Jun 7, 2012

    I'm new here so I apologize if this is considered poor etiquette, but I'm just commenting to 'bump' this issue. My search for a way to accomplish exactly this functionality led me here, and it seems a patch was offered but no action has been taken on it for over a year now. Alas, it seems I must write a custom Action handler instead.

    @bethard
    Copy link
    Mannequin

    bethard mannequin commented Jul 22, 2012

    The tests look like they're testing the right things, but the tests should instead be written like the rest of the argparse tests. For example, look at TestOptionalsNargs3 and TestPositionalsNargs2. You could write your tests to look something like those, e.g.

    class TestOptionalsNargs1_3(ParserTestCase):
    
        argument_signatures = [Sig('-x', nargs=(1, 3))]
        failures = ['a', '-x', '-x a b c d']
        successes = [
            ('', NS(x=None)),
            ('-x a', NS(x=['a'])),
            ('-x a b', NS(x=['a', 'b'])),
            ('-x a b c', NS(x=['a', 'b', 'c'])),
        ]

    Also, a complete patch will need to document the new feature in the Python documentation, among other things. See the details described here:

    http://docs.python.org/devguide/patch.html#preparation

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented May 8, 2013

    Wouldn't it be simpler to use the re {m,n} notation to grab the appropriate number of arguments?

    In ArgumentParser _get_nargs_pattern we could add:

    + # n to m arguments, nargs is re like {n,m}
    + elif is_mnrep(nargs):
    + nargs_pattern = '([-A]%s)'%nargs

        # all others should be integers
    

    where is_mnrep() tests that the nargs string looks like '{m,n}' (or '{,n}' or '{m,}'). The resulting nargs_pattern will look like

    '([-A]{m,n})'
    

    In _format_args() a similar addition would be:

    + elif is_mnrep(action.nargs):
    + result = '%s%s' % (get_metavar(1)[0], action.nargs)

    'FOO{2,5}'

    It would take more code to generate

    FOO FOO [FOO [FOO [FOO]]]

    A matching programmer input would be:

       parser.add_argument('Foo', nargs='{2,5}')

    though it wouldn't be much work to also translate a tuple.

       parser.add_argument('Foo', nargs=(2,5))

    I'll try to test this implementation against wm's tests.

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented May 9, 2013

    I think this patch should build on http://bugs.python.org/issue9849, which seeks to improve the error checking for nargs. There, add_argument returns an ArgumentError if the nargs value is not a valid string, interger, or it there is mismatch between a tuple metavar and nargs.

    This range nargs should be tested at the same time, and return a ArgumentError if possible.

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented May 9, 2013

    This patch adds this nargs='{m,n}' or nargs=(m,n) feature.

    It builds on the better nargs error message patch in http://bugs.python.org/msg187754

    It includes an argparse.rst paragraph, changes to argparse.py, and additions to test_argparse.py.

    The tests include those proposed by wm, rewritten to use the ParserTestCase framework where possible. I did not give a lot of thought to test names. The coverage could also use further thought.

    As WM noted some range cases are the equivalent to existing nargs options ('{1,}'=='+'). I did not attempt to code or test such equivalences. Since the '{0,}' form uses regex matching just like '*',
    I don't think there is any advantage to making such a translation.

    I convert the tuple version (m,n) to the re string '{m,n}' during the add_argument() testing. So it is the string form that is added to the parser action. This is also the format that appears in usage.

    The documentation paragraph is:

    '{m,n}'. m to n command-line arguments are gathered into a list. This is modeled on the Regular Expression use. '{,n}' gathers up to n arguments. '{m,}' gathers m or more. Thus '{1,}' is the equivalent to '+', and '{,1}' to '?'. A tuple notation is also accepted, '(m,n)', '(None,n)', '(m,None)'. For example:

        >>> parser = argparse.ArgumentParser(prog='PROG')
        >>> parser.add_argument('--foo', nargs='{2,4}')
        >>> parser.parse_args('--foo a b c'.split())
        Namespace(foo=['a', 'b', 'c'])
        >>> parser.parse_args('--foo a'.split())
        usage: PROG [-h] [--foo FOO{2,4}]
        PROG: error: argument --foo: expected {2,4} arguments

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Nov 30, 2013

    With a minor tweak to argparse._is_mnrep(), nargs='{3}' would also work. This means the same as nargs=3, so it isn't needed. But it is consistent with the idea of accepting Regular Expression syntax where it makes sense. nargs='{2,3}?' also works, though I think that's just the same as nargs=2.

    @alclarks
    Copy link
    Mannequin

    alclarks mannequin commented Nov 3, 2019

    Hi, I'm a new contributor looking for issues to work on. This looks like a nice enhancement, would it be a good idea for me to update the patch and raise a pull request?

    @shihai1991
    Copy link
    Member

    I think the answer is 'yes'(It's over 8 years~).
    And tons of argparse'work need to be finished;)

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Nov 3, 2019

    A couple of quick observations:

    • this is one of the first patches that I worked on, so the details aren't fresh in my mind. A glance at my latest patch show that this isn't a trivial change.

    • nargs changes affect the input handling, the parsing, help and usage formatting, and error formatting. As a result, nargs are referenced in several places in the code. That complicates maintenance and the addition of new features. Help formatting is particularly brittle; just look at the number of issues that deal with 'metavar' to get a sense of that.

    • At one point I tried to refactor the code to consolidate the nargs handling in a few functions. I don't recall if I posted that as a patch.

    • The first posts on this topic suggested a (n,m) notation; I proposed a regex like {n,m}. There wasn't any further discussion.

    • Note that the initial posts were in 2011 when Steven (the original author) was involved. I posted in 2013. There hasn't been any further action until now. I don't recall much interest in this topic on Stackoverflow either. So my sense is that this isn't a pressing issue.

    • It's easy to test for this range after parsing, with '*' or '+' nargs. So the main thing this patch adds is in the help/usage display. It doesn't add significant functionality.

    • cross links:

    https://bugs.python.org/issue9849 - Argparse needs better error handling for nargs

    https://bugs.python.org/issue16468 - argparse only supports iterable choices (recently closed)

    @alclarks
    Copy link
    Mannequin

    alclarks mannequin commented Nov 4, 2019

    I've had a look at the most recent patch and the code surrounding it, and I would be happy to take on the work to update the code and testing.

    However,

    • It's easy to test for this range after parsing, with '*' or '+' nargs. So the main thing this patch adds is in the help/usage display. It doesn't add significant functionality.

    I didn't initially consider this.

    I'd still be happy to finish this off, but if the general feeling is that contribution time would be better spent elsewhere then that's also fine.

    @rhettinger
    Copy link
    Contributor

    Do we have examples of real world APIs that actually need this functionality? Offhand, I don't recall having worked with any command-line tool that would accept "at least two but no more than four filenames" for example.

    Given that this isn't trivial to implement, we should take a moment to evaluate whether users actually need it. We had eight years since the original proposal without anyone chiming in to say, "yes, I need this".

    @shihai1991
    Copy link
    Member

    Could we close some bpo with a long history? If some user need this feature in future we could reopen it.

    @alclarks
    Copy link
    Mannequin

    alclarks mannequin commented Nov 15, 2019

    Weighing up how little demand there seems to be for this, and how easy it is to achieve the same effect with post-processing within a hypothetical script that happens to need it - I agree with closing it.

    @calestyo
    Copy link
    Mannequin

    calestyo mannequin commented Jul 10, 2020

    Next to code readability, there's IMO one could reason to properly support this would be a clean and easy way to get proper help strings for such options.

    Of course I can do something like:
    parser = argparse.ArgumentParser()
    parser.add_argument("--foo", nargs="+", help="Mae govannen", metavar=("bar", "baz"))
    args = parser.parse_args()

    and later check that, say, only 2 arguments are allowed.

    But the help text will be an ugly:

    $ python3 f.py --help
    usage: f.py [-h] [--foo bar [baz ...]]

    optional arguments:
    -h, --help show this help message and exit
    --foo bar [baz ...] Mae govannen

    indicating that >1 options were allowed.

    @uniqueg
    Copy link
    Mannequin

    uniqueg mannequin commented Mar 20, 2021

    Given that people were asking for real-world use cases, here's one: high-throughput sequencing, e.g., in RNA-Seq (https://en.wikipedia.org/wiki/RNA-Seq), typically yields either one or two output files, depending on the type of the sequencing library. As the processing of these library types is very similar, bioinformatics tools dealing with these inputs thus typically have APIs that take exactly 1 or 2 files as inputs. As there is quite a wide range of tools for dealing with such inputs, several of which implemented in Python, I believe there is indeed an interest in this functionality. On a more conceptual note: it is also consistent with similar and often-used functionality in regexes, from which, I suppose, the '+' and '*' notation is borrowed.

    Currently implementing such a tool, I ran into the argparse limitation described here: either I (a) use a positional param with nargs=1 for the first file and define an optional param for the second file (inconsistent, non-intuitive and semantically incorrect API, because if there IS a second file, it is not really optional), (b) use nargs='+', do the admittedly simple post-processing/validation and either ignore keep the auto-generated usage string(wrong/misleading), hardcode the correct usage string (maintenance burden because of several optional params) or apply this patch (or just the auto-usage generation function), which seems rather expensive, or (c) have the user pass the second file in one string, separated by a comma or similar (also not very intuitive and needs some checks to ensure that the filename/s don't actually include commas).

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Apr 25, 2021

    Post parsing testing for the correct number of strings is the easy part.

    It's the auto-generate usage that's harder to do right, especially if we wanted to enable the metavar tuple option. Clean usage for '+' is messy enough.

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants