swansonk14 / typed-argument-parser

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

`tapify` support positional-only arguments #133

Closed kddubey closed 6 months ago

kddubey commented 6 months ago

tapify currently doesn't work when the signature contains positional-only arguments.

For data models, AFAIK, there isn't such a thing as a positional-only argument. So the change only needs to be applied to (non-data-model) functions or classes.

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.09%. Comparing base (821c08c) to head (ddd177d).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #133 +/- ## ========================================== + Coverage 94.01% 94.09% +0.07% ========================================== Files 5 5 Lines 685 694 +9 ========================================== + Hits 644 653 +9 Misses 41 41 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kddubey commented 6 months ago

@martinjm97 Does Tap ever result in positional arguments in the command line?

Looks like the answer is no. ```python # script.py from tap import Tap class MyTap(Tap): x: int if __name__ == "__main__": print(MyTap().parse_args()) ``` Command: ``` $ python script.py -h usage: script.py --x X [-h] options: --x X (int, required) -h, --help show this help message and exit ``` `--x` is a required option, not a positional argument. So `python script.py 1` doesn't work but `python script.py --x 1` does work.

Given this, maybe it's too early to allow tapify to support positional command line arguments (via the all_args_named=False setting in this PR). But up to you and @swansonk14 to decide :-)

There are some complications in supporting positional command line arguments. Namely, handling a Tap object where many arguments are collection types. And for tapify: handling a class/function parameter which is positional-only but not required (i.e., it has a default value), or a parameter which is not positional-only but is required.

I lean towards keeping the current behavior for now: no positional command line arguments / all options. Maybe you can consider it in the future if many people want it.

Regardless, I'll make the fix to allow tapify to work when class_or_function has positional-only parameters.

kddubey commented 6 months ago
Demo you can run and mess with ```python # script.py from tap import tapify def foo(num1: int, num2: float, /, bar: str, kwarg1: list[int] = None) -> None: """Foo does nothing. :param num1: num1 is a number :param num2: also a number :param bar: foobar :param kwarg1: something, defaults to None """ print(locals()) if __name__ == "__main__": tapify(foo) ``` Help message: ``` (tap) ➜ typed-argument-parser git:(support-positional-only-arguments) ✗ python script.py -h usage: script.py --num1 NUM1 --num2 NUM2 --bar BAR [--kwarg1 [KWARG1 ...]] [-h] Foo does nothing. options: --num1 NUM1 (int, required) num1 is a number --num2 NUM2 (float, required) also a number --bar BAR (str, required) foobar --kwarg1 [KWARG1 ...] (list[int], default=None) something, defaults to None -h, --help show this help message and exit ``` Run it: ``` (tap) ➜ typed-argument-parser git:(support-positional-only-arguments) ✗ python script.py --num1 1 --num2 2.1 --bar "foo" {'num1': 1, 'num2': 2.1, 'bar': 'foo', 'kwarg1': None} ```
martinjm97 commented 6 months ago

Thank you! It looks like you solved the issue! --JK

kddubey commented 6 months ago

You're welcome :-)

Though to be clear, this PR only addresses one part of #100: this PR makes tapify work (instead of error out) when the input has positional-only parameters. It doesn't address the main part of the issue, which is having tapify result in command line positional args.

Lmk if you'd like that feature/enhancement as well. I jotted down some of its complications in this comment.

martinjm97 commented 6 months ago

I see. I think addressing the challenges you mentioned in https://github.com/swansonk14/typed-argument-parser/pull/133#issuecomment-2055672988 would be quite challenging. As a result, I'd advocate for stopping tapify from erroring out when given a function with positional arguments and general support for positional arguments in tapify as separate issues.

Kyle and I discussed some of the challenges and didn't have great ideas for what to do for positional arguments that are collections. It's not clear where one list ends and the next one starts! In general, it's not clear to me that we'd like Tap classes to represent positional arguments natively (rather than specifying it explicitly in configure).

There'd probably be additional complications with optional positional list arguments (argparse reference).

I lean towards keeping the current behavior for now: no positional command line arguments / all options. Maybe you can consider it in the future if many people want it.

I agree!

I think you've resolved the original issue to the level where tapify is more generally usable. I think further improvement may not be worth it yet.