swansonk14 / typed-argument-parser

Typed argument parser for Python
MIT License
497 stars 40 forks source link

Subparsers are not typed. #69

Open pvalsecc opened 2 years ago

pvalsecc commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

kavinvin commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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)
toburger commented 2 weeks ago

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?