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

Subparsers are not typed. #69

Open
pvalsecc opened this issue Jan 24, 2022 · 7 comments
Open

Subparsers are not typed. #69

pvalsecc opened this issue Jan 24, 2022 · 7 comments
Labels
duplicate This issue or pull request already exists enhancement New feature or request

Comments

@pvalsecc
Copy link

pvalsecc commented Jan 24, 2022

Unless I've missed something, the usage of subparsers annihilates the advantages of this library.

If I take your example:

class SubparserA(Tap):
    bar: int  # bar help

class Args(Tap):
    foo: bool = False  # foo help

    def configure(self):
        self.add_subparsers(help='sub-command help')
        self.add_subparser('a', SubparserA, help='a help')

args = Args().parse_args('--foo a --bar'.split())
print(args.foo)  # <--- OK, this is typed
print(args.bar)  # <--- not typed!!!!!

Can't this be more like that?

class SubparserA(Tap):
    bar: int  # bar help

class Args(Tap):
    foo: bool = False  # foo help
    a: Optional[SubParserA] = None  # a help

    def configure(self):
        self.add_subparsers(help='sub-command help')

args = Args().parse_args('--foo a --bar'.split())
print(args.a.bar)  # <--- now, it's typed
@pvalsecc
Copy link
Author

Example of test:

    def test_typed(self):
        class SubparserA(Tap):
            bar: int  # bar help

        class SubparserB(Tap):
            baz: Literal['X', 'Y', 'Z']  # baz help

        class ArgsTyped(Tap):
            foo: bool = False  # foo help
            a: Optional[SubparserA] = None  # a help
            b: Optional[SubparserB] = None  # b help

            def configure(self):
                self.add_subparsers(help='sub-command help')

        args = ArgsTyped().parse_args([])
        self.assertFalse(args.foo)
        self.assertIsNone(args.a)
        self.assertIsNone(args.b)

        args = ArgsTyped().parse_args(['--foo'])
        self.assertTrue(args.foo)
        self.assertIsNone(args.a)
        self.assertIsNone(args.b)

        args = ArgsTyped().parse_args('a --bar 1'.split())
        self.assertFalse(args.foo)
        self.assertIsNotNone(args.a)
        self.assertEqual(args.a.bar, 1)
        self.assertIsNone(args.b)

        args = ArgsTyped().parse_args('--foo b --baz X'.split())
        self.assertTrue(args.foo)
        self.assertIsNone(args.a)
        self.assertIsNotNone(args.b)
        self.assertEqual(args.b.baz, 'X')

        sys.stderr = self.dev_null

        with self.assertRaises(SystemExit):
            ArgsTyped().parse_args('--baz X --foo b'.split())

        with self.assertRaises(SystemExit):
            ArgsTyped().parse_args('b --baz X --foo'.split())

        with self.assertRaises(SystemExit):
            ArgsTyped().parse_args('--foo a --bar 1 b --baz X'.split())

@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

I wasn't sure where you wanted comments. If there's a better place to do so, please let me know :)

The proposed approach is an interesting idea, though it seems a little strange to me to mix fields, some of which are arguments and some of which are a collection of mutually-exclusive objects corresponding to subparsers.

Have you considered taking advantage of subclassing? i.e., would it work if subparsers subclass the base args class, something like this:

        class ArgsTyped(Tap):
            foo: bool = False  # foo help

            def configure(self):
                subparser = self.add_subparsers(help='sub-command help')

        class SubparserA(ArgsTyped):    # a
            bar: int  # bar help

        class SubparserB(ArgsTyped):    # b
            baz: Literal['X', 'Y', 'Z']  # baz help

        class SubparserC(ArgsTyped):    # c
            pass

and to access the subparser args:

args = Args().parse_args('--foo a --bar'.split())
if isinstance(args, SubparserB):
    print(args.bar)  # <--- now, it's typed

I am not really sure about using a comment for the subparser name, though the alternative would be to use subparsers.add_parser for each one.

You will see I included an approach for subparsers with no arguments. I have a project where the command has a number of "actions" that sometimes have options, sometimes not. The other option would be to use e.g., self.add_subparsers(dest='action', required=True, help='sub-command help'), but that only helps for no arguments so it's probably cleaner to use isinstance consistently.

Edit: see my original comments at #65 (comment) I missed that comments were requested in each of the relevant issues.

@kavinvin
Copy link

kavinvin commented Mar 11, 2022

I'd love having typed subparsers as well, but I have another opinion a bit different from @pvalsecc regarding the type:

a: Optional[SubparserA]
b: Optional[SubparserB]

Having this type is a bit inaccurate that it allows 4 possible values:

a = None, b = None
a = SubparserA, b = None
a = None, b = SubparserB
a = SubparserA, b = SubparserB

When in fact, we can only have two three:

a = None, b = None
a = SubparserA, b = None
a = None, b = SubparserB

Instead, we can use union type to represent SubparserA | SubparserB. Therefore, SubparserA and SubparserB types can also be suggested correctly after pattern matching.

class SubparserA(Tap):
    foo: str

class SubparserB(Tap):
    bar: str

class RootParser(Tap):
    def configure(self) -> None:
        self.add_subparsers(help="sub-command help")
        self.add_subparser("foo", SubparserA)
        self.add_subparser("bar", SubparserB)

    def process_args(self) -> None:
        self.command: SubparserA | SubparserB | None = cast(SubparserA | SubparserB | None, self)

args = RootParser().parse_args()

args.command type will be SubparserA | SubparserB | None

The drawback of this approach is that Python pattern matching is, IMO, not that fun to use and can be verbose sometimes.

@illeatmyhat
Copy link

illeatmyhat commented Aug 24, 2022

you could also have multiple __main__ modules instead of using a single root parser.
In fact, if I insisted on having """the full CLI experience""" I'd parse the input in a single __main__ function, use the very first token to determine which parser I'm using, and then pass the remainder of the string into the parser.

@LiraNuna
Copy link

LiraNuna commented Sep 24, 2022

Another issue that I would love to insert here that I feel related, is that process_args is NOT called on the subparsers!

from tap import Tap


class SubparserA(Tap):
    bar: bool
    baz: bool

    def process_args(self) -> None:
        if self.bar and self.baz:
            raise TypeError('cannot use bar and baz together')


class Args(Tap):
    foo: bool = False  # foo help

    def configure(self):
        self.add_subparsers(required=True, help='sub-command help')
        self.add_subparser('a', SubparserA, help='a help')


# call with `./script.py a --bar --baz` to reproduce
if __name__ == '__main__':
    args = Args().parse_args()
    print(args)

@swansonk14 swansonk14 added duplicate This issue or pull request already exists enhancement New feature or request and removed bug Something isn't working labels Jun 25, 2023
@toburger
Copy link

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

Any news on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants