typed-argparse / typed-argparse

💡 write type-safe and elegant CLIs with a clear separation of concerns.
https://typed-argparse.github.io/typed-argparse/
MIT License
28 stars 6 forks source link

Subparser command name is not constrained by Literal type definition #9

Closed pdc1 closed 1 year ago

pdc1 commented 2 years ago

There is a nice example for using Literal[] typing for arguments (https://github.com/bluenote10/typed_argparse#convenience-functionality-to-map-literalenum-to-choices) but there does not appear to be a similar approach for subparser command definitions.

Would it be possible to add something similar in this module?

Conceptually I'm thinking of something similar to the choices approach, only for the subparser, e.g. something like:

subparsers = parser.add_subparsers(
        help="Available sub commands",
        dest="mode",
        required=True,
        choices=MyArgs.get_choices_from("mode"),
    )

If the project had the approach shown in https://github.com/bluenote10/typed_argparse#support-for-union-useful-for-subcommand-parsing, then perhaps you'd need a union of the different mode definitions.

Alternately it would be nice if validate() could catch cases where the Literal[...] definition did not match the argument passed to subparsers.add_parser().

This module has been very helpful, thanks for making it available!

bluenote10 commented 2 years ago

Doesn't that already work? As far as I can see argparse doesn't infer the "choices" from an argument passed into add_subparsers but rather from the first arguments passed to subparsers.add_parser. To give a full example:

import argparse
import sys
from typing import List, Literal, Union

from typed_argparse import TypedArgs, WithUnionType

class CommonArgs(TypedArgs):
    verbose: bool

class ArgsFoo(CommonArgs):
    mode: Literal["foo"]
    file: str

class ArgsBar(CommonArgs):
    mode: Literal["bar"]
    src: str
    dst: str

Args = Union[ArgsFoo, ArgsBar]

def parse_args(args: List[str] = sys.argv[1:]) -> Args:
    parser = argparse.ArgumentParser()
    parser.add_argument("--verbose", action="store_true", help="Verbose")
    subparsers = parser.add_subparsers(
        help="Available sub commands",
        dest="mode",
        required=True,
    )

    parser_foo = subparsers.add_parser("foo")
    parser_foo.add_argument("file", type=str)

    parser_bar = subparsers.add_parser("bar")
    parser_bar.add_argument("--src", required=True)
    parser_bar.add_argument("--dst", required=True)

    return WithUnionType[Args].validate(parser.parse_args(args))

if __name__ == "__main__":
    parse_args()

Behaves as follows:

image

There is just the minor redundancy that subparsers.add_parser has to duplicate the value from the Literal. It might be possible to come up with a helper function that allows to reference the type + the field to get to the literal value. Something like subparsers.add_parser(get_literal_value_of(Foo, "mode")). Is that what you are looking for?

pdc1 commented 2 years ago

I was looking for protection from typos on the subparsers.add_parser() argument, but I must have confused myself because playing around with the example I do get a runtime validation error if I change "bar" to subparsers.add_parser("baz") for example.

Part of what I was looking for was a typing error, i.e. so mypy/etc would flag the above, but I don't see any reasonable way for it to know the class property and the argument to add_parser() are related.

To that end, something like you suggested would help, though to be honest there are a number of other minor redundancies (like property names matching add_argument() calls) that I'm not sure it's really necessary.

The main thing I was looking for was a failsafe against typos in validate(), and it is already there! Sorry for the false alarm :)

bluenote10 commented 2 years ago

To that end, something like you suggested would help, though to be honest there are a number of other minor redundancies (like property names matching add_argument() calls) that I'm not sure it's really necessary.

That's also my impression. I'll leave the issue open as a reminder just in case I come across an elegant idea...

The main thing I was looking for was a failsafe against typos in validate(), and it is already there! Sorry for the false alarm :)

No worries!

bluenote10 commented 1 year ago

I'd close this assuming the issue has been resolved? Feel free to re-open / ask more if needed.