typed-argument-parser icon indicating copy to clipboard operation
typed-argument-parser copied to clipboard

Subparsers are not typed.

Open pvalsecc opened this issue 2 years ago • 6 comments

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 avatar Jan 24 '22 13:01 pvalsecc

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())

pvalsecc avatar Jan 24 '22 14:01 pvalsecc

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

swansonk14 avatar Feb 06 '22 02:02 swansonk14

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 https://github.com/swansonk14/typed-argument-parser/issues/65#issuecomment-1003427983 I missed that comments were requested in each of the relevant issues.

pdc1 avatar Feb 11 '22 17:02 pdc1

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.

kavinvin avatar Mar 11 '22 16:03 kavinvin

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.

illeatmyhat avatar Aug 24 '22 00:08 illeatmyhat

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)

LiraNuna avatar Sep 24 '22 15:09 LiraNuna