Skip to content

Conversation

@kmvanbrunt
Copy link
Member

This PR is large and breaks stuff with the goal of simplifying our API.

Completely overhauled how argparse argument tab completion is defined.

Instead of:

setattr(parser.add_argument('my_arg', help='help stuff'),
            ACTION_ARG_CHOICES, ('path_complete',))

You do:

parser.add_argument('my_arg', help='help stuff', completer_method=path_complete)

Also patched argparse functions to enable nargs ranges on all parsers and not just the cmd2 parser.

The changes are documented in the header of argparse_custom.py.

kmvanbrunt added 30 commits July 1, 2019 12:30
…tings like enabling tab completion and providing choice generating functions
Rename ACArgumentParser to Cmd2ArgParser
@kmvanbrunt kmvanbrunt reopened this Jul 12, 2019
@kmvanbrunt
Copy link
Member Author

PR is open again

@kotfu
Copy link
Member

kotfu commented Jul 15, 2019

This is much nicer than what we have now. I like it a lot. A couple of thoughts:

  • Should we name our custom ArgParser class ArgumentParser? Then you would either use cmd2.ArgumentParser or argparse.ArgumentParser.
  • The custom ArgParser class over-rides several private methods of argparse.ArgumentParser. This makes me nervous. It could lead to a situation where a minor python revision requires us to do a x.0.0 version release.
  • These changes to ArgumentParser are potentially interesting enough to split out into a separate library. Have you thought about it?

@kmvanbrunt
Copy link
Member Author

kmvanbrunt commented Jul 15, 2019

@kotfu Thanks for the feedback. Here are my thoughts.

  1. I'm OK renaming the parser to cmd2.ArgumentParser. @tleonhardt What are your thoughts?
  2. I too am a little uncomfortable overriding private methods of ArgumentParser. Hopefully we won't experience any problems since most of the functions are merely wrappers that handle our custom data before calling down to the original. Also, it doesn't look like argparse is changed that often.
  3. I hadn't considered splitting any of the ArgumentParser changes into another library. What are you envisioning?

@tleonhardt
Copy link
Member

tleonhardt commented Jul 16, 2019

I'm fine renaming our custom parser to ArgumentParser and I'm also fine leaving it as ArgParser - I see advantages and disadvantages either way. The advantage of naming it slightly differently is it emphasizes the differences from argparse.ArgumentParser. The advantage of naming it the same is it emphasizes the similarities.

If people use it via the import of cmd like cmd2.ArgumentParser or cmd2.ArgParser, then either is OK and perhaps ArgumentParser is better. However, if people use it via from cmd2 import ArgumentParser where at use time is is just ArgumentParser, I fear that it might confuse some people if they don't realize it is a custom cmd2.ArgumentParser and not argpasre.ArgumentParser. So I slightly favor leaving it as ArgParser. But I don't feel strongly about this at all.

tleonhardt
tleonhardt previously approved these changes Jul 16, 2019
@kmvanbrunt
Copy link
Member Author

I'll merge this for now and give some thought to the ArgParser/ArgumentParser name choice.

@kmvanbrunt kmvanbrunt merged commit 94b424e into master Jul 16, 2019
@kmvanbrunt kmvanbrunt deleted the auto_completer_refactor branch July 16, 2019 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants