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

ArgumentParser inconsistent with parse_known_args #60346

Closed
idank mannequin opened this issue Oct 5, 2012 · 13 comments
Closed

ArgumentParser inconsistent with parse_known_args #60346

idank mannequin opened this issue Oct 5, 2012 · 13 comments
Assignees
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 new features, bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@idank
Copy link
Mannequin

idank mannequin commented Oct 5, 2012

BPO 16142
Nosy @bitdancer, @osandov, @aeros, @benedwards14
Files
  • aparse.py
  • dashku.patch
  • dashku.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 = None
    created_at = <Date 2012-10-05.14:53:21.479>
    labels = ['easy', 'type-bug', 'library']
    title = 'ArgumentParser inconsistent with parse_known_args'
    updated_at = <Date 2019-11-12.21:46:41.296>
    user = 'https://bugs.python.org/idank'

    bugs.python.org fields:

    activity = <Date 2019-11-12.21:46:41.296>
    actor = 'LewisGaul'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2012-10-05.14:53:21.479>
    creator = 'idank'
    dependencies = []
    files = ['27434', '29910', '30056']
    hgrepos = []
    issue_num = 16142
    keywords = ['patch', 'easy']
    message_count = 10.0
    messages = ['172088', '172089', '172091', '172092', '173492', '173640', '181167', '187212', '188038', '355469']
    nosy_count = 11.0
    nosy_names = ['bethard', 'r.david.murray', 'idank', 'paul.j3', 'Sam.Breese', 'Radu.Ciorba', 'Andrei.Vereha', 'osandov', 'kynikos', 'aeros', 'benedwards14']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'needs patch'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue16142'
    versions = ['Python 2.7', 'Python 3.3']

    Linked PRs

    @idank
    Copy link
    Mannequin Author

    idank mannequin commented Oct 5, 2012

    When known and unknown options are given together in the same option string (e.g. -xy) then ArgumentParser behaves in a strange way:

    • if the known option is given first (so -k is known and the parser is fed with ['-ku']) then the parsing aborts with "error: argument -k/--known: ignored explicit argument 'u'"

    • if the unknown option is given first then both options are treated as unknown and returned in the list of remaining arguments.

    This makes it impossible to use parse_known_args for its intended purpose because every single letter option might be interleaved with other unknown options.

    I attached a test script that demonstrates this.

    @idank idank mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 5, 2012
    @bitdancer
    Copy link
    Member

    Looks like parse_known_args needs to be taught to not treat unknown text following an option as an argument if the option does not take an argument. That would be in keeping with its mission, I think :)

    There will still probably be ambiguous cases that will trip you up, but that should help.

    @bitdancer bitdancer added the easy label Oct 5, 2012
    @idank
    Copy link
    Mannequin Author

    idank mannequin commented Oct 5, 2012

    Yes that'd fix the known option before unknown but not the other way around.

    @bitdancer
    Copy link
    Member

    Right. I didn't read what you wrote carefully enough. Clearly parse_known_args is buggy here.

    @SamBreese
    Copy link
    Mannequin

    SamBreese mannequin commented Oct 22, 2012

    Writing a patch now. Should be ready in a few hours.

    @SamBreese
    Copy link
    Mannequin

    SamBreese mannequin commented Oct 23, 2012

    Make that a few days. I fixed the case where the known arg is first, but not the other one. Will get to it soon, hopefully.

    @RaduCiorba
    Copy link
    Mannequin

    RaduCiorba mannequin commented Feb 2, 2013

    "- if the unknown option is given first then both options are treated as unknown and returned in the list of remaining arguments."
    I don't think this case is correct behaviour. There is no way to determine if u accepts arguments or not. If nargs for -u is "?" or more, the current behavior is correct since everything after -u would be an argument for this option.

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Apr 17, 2013

    parser = argparse.ArgumentParser()
        parser.add_argument('-k','--known',action='store_true')
        print(parser.parse_known_args(['-k','-u']))
        print(parser.parse_known_args(['-ku']))
        print(parser.parse_known_args(['-uk']))

    I think you want these 3 cases to produce the same output:

    (Namespace(known=True), ['-u'])
    

    With the attached patch, '-ku' produces the same result as '-k -u'. Instead of raising the "ignored explicit argument 'u'" error, if puts '-u' in the 'extras' list. 'parse_args' then raises a "error: unrecognized arguments: u" error.

    That's an easy change, and does not break anything in the 'test_argparse.py' file. But keep in mind that this test file mostly uses 'parse_args', and usually it ignores the failure messages.

    Getting '-uk' to work this way would be much harder. While it isn't obvious from the documentation, '-uk' is a valid option string, and '-u' is a valid abbreviation. Notice in 16.4.4.1. of the documentation, the difference between long and short options is based on the number of characters, not whether there it starts with '-' or '--'. So identifying what is wrong with '-uk' would require ambiguous reasoning.

    I wonder what optparse does.

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Apr 29, 2013

    Correction: The patch I gave in the last message produces:

        >>> parser.parse_known_args(['-ku'])
        (Namespace(known=False), ['u'])

    It doesn't take action on the '-k', and puts 'u' in extras, not '-u'.

    This new patch gets it right:

        >>> parser.parse_known_args(['-ku'])
        (Namespace(known=True), ['-u'])

    We need more test cases, including ones that work as expected with optparse or other unix parsers.

    @benedwards14
    Copy link
    Mannequin

    benedwards14 mannequin commented Oct 27, 2019

    Hey this has been open for quite a while, is there anything that needs finishing off?

    @furkanonder
    Copy link
    Sponsor Contributor

    The PR is ready for review.

    @AA-Turner AA-Turner removed the easy label Sep 27, 2023
    @serhiy-storchaka
    Copy link
    Member

    For -k=unknown it gives (Namespace(known=True), ['-u', '-n', '-n', '-o', '-w', '-n']).

    @serhiy-storchaka
    Copy link
    Member

    I tested different corner cases, and neither proposed patches, nor PR #103197 handle them correctly.

    If -k and -n are known flags:

    -kunknown should recognize -k and leave -unknown, because we do not know whether unknown option -u takes argument or not.
    -k=unknown should be an error, because -k has an explicit argument here.
    -knew should recognize -k and -n and leave -ew.
    -k-new should be an error, because a single-dash option cannot be --, and it cannot leave a double-dash option --new.
    -kn-ew should be an error, for same reason.
    -kn=ew should be an error, because -n has an explicit argument here.
    -kne-w should recognize -k and -n and leave -e-w.
    -kne=w should recognize -k and -n and leave -e=w.

    #114180 handles all these corner cases (and maybe more). It is much more complex than the original patch.

    @serhiy-storchaka serhiy-storchaka self-assigned this Jan 17, 2024
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 19, 2024
    …r.parse_known_args() (pythonGH-114180)
    
    (cherry picked from commit e47ecbd)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 19, 2024
    …r.parse_known_args() (pythonGH-114180)
    
    (cherry picked from commit e47ecbd)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    @serhiy-storchaka serhiy-storchaka added 3.11 only security fixes 3.12 bugs and security fixes 3.13 new features, bugs and security fixes labels Feb 19, 2024
    serhiy-storchaka added a commit that referenced this issue Feb 19, 2024
    …er.parse_known_args() (GH-114180) (GH-115674)
    
    (cherry picked from commit e47ecbd)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    serhiy-storchaka added a commit that referenced this issue Feb 19, 2024
    …er.parse_known_args() (GH-114180) (GH-115675)
    
    (cherry picked from commit e47ecbd)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    woodruffw pushed a commit to woodruffw-forks/cpython that referenced this issue Mar 4, 2024
    diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes 3.12 bugs and security fixes 3.13 new features, bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    Status: Doc issues
    Development

    No branches or pull requests

    4 participants