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

parse_args should return the chosen sub parser object #65

Open
JasonGilholme opened this issue Nov 8, 2021 · 4 comments
Open

parse_args should return the chosen sub parser object #65

JasonGilholme opened this issue Nov 8, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@JasonGilholme
Copy link

JasonGilholme commented Nov 8, 2021

Hi,

I'm using sub parsers and I assumed that the Tap for the chosen sub parser would be returned from parse_args() however the parent parser is actually returned. The motivation for having the sub parser returned is as follows:

  • allow additional functionality to be loaded on to the parser
  • get code completion for the sub parser as you do for the parent parser
  • determine which sub parser was chosen in a manner that typing can understand.

For example:

class SubParserA(Tap):

    foo: str

    def do_foo(self):
        # Do something with self.foo here


class SubParserB(Tap)

    bar: str

    def do_bar(self):
        # Do something with self.bar here


class MainParser(Tap):

    def configure(self) -> None:
        self.add_subparsers()
        self.add_subparser('a', SubParserA)
        self.add_subparser('b', SubParserB)


if __name__ == '__main__':
    main_parser = MainParser()
    args = main_parser.parse_args()

    if isinstance(args, SubParserA):
        args.do_foo()
    elif isinstance(args, SubParserB):
        args.do_bar()

Would be great to hear your thoughts!

Cheers
Jase

@martinjm97
Copy link
Collaborator

Hi @JasonGilholme,

Thank you for raising this issue! We agree that the current behavior is not ideal. We've already put some thought into this problem #17 (comment), but we're still thinking about it.

For future us and perhaps others, we'll leave some runnable code that breaks (heavily inspired by your code) when you run python file.py a --foo biz:

from tap import Tap

class SubParserA(Tap):
    foo: str

    def do_foo(self):
        pass

class SubParserB(Tap):
    bar: str

    def do_bar(self):
        pass

class MainParser(Tap):

    def configure(self) -> None:
        self.add_subparsers()
        self.add_subparser('a', SubParserA)
        self.add_subparser('b', SubParserB)


if __name__ == '__main__':
    main_parser = MainParser()
    args = main_parser.parse_args()

    args.do_foo()

Best,
Jesse and Kyle

@pdc1
Copy link

pdc1 commented Dec 31, 2021

I would like to second this. With the current implementation I am not sure how this is supposed to work, but in the example you provide, the code to access args.foo or args.bar gives a type exception (VS Code/pylance). Which makes sense since they are defined in separate classes.

What I expected:

from tap import Tap

class MainParser(Tap):
    shared: str

    def configure(self) -> None:
        self.add_subparser('a', SubParserA)
        self.add_subparser('b', SubParserB)

class SubParserA(MainParser):
    foo: str

class SubParserB(MainParser):
    bar: str

if __name__ == '__main__':
    args = MainParser().parse_args()
    if isinstance(args, SubParserA):
        print(args.shared)
        print(args.foo)

That code does not show any errors in the editor, but when run goes into an infinite loop which seems to involve _get_class_variables and _get_from_self_and_super. And without the subclasses, the "isinstance" part will never be true. Is there a trick for that?

As an aside, it would be nice to allow "None" as the subparse class, for subparsers without additional arguments. I created a NoArgs object instead, so maybe that's good enough, but maybe include as an example?

@swansonk14
Copy link
Owner

Hi all,

We’re working on sketching a plan for a Tap version 2 release. We have a sketch of the solution to a number of issues including this one. This includes a breaking change to the treatment of subparsers and a different way that we treat comments to support argument groups. We’re excited to start the implementation and would love feedback and recommendations from everyone. We’re so appreciative of all of the wonderful pull requests, issues, and ideas from all of you!

Best,
Kyle and Jesse

@pdc1
Copy link

pdc1 commented Feb 11, 2022

See my comments at #69 (comment). Thanks!

@swansonk14 swansonk14 added enhancement New feature or request invalid This doesn't seem right and removed enhancement New feature or request invalid This doesn't seem right labels Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants