tapify support positional-only arguments

[kddubey]

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files

@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kddubey]

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

Looks like the answer is no.

# 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]

Demo you can run and mess with

# 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]

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

[kddubey]

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]

I see. I think addressing the challenges you mentioned in #133 (comment) 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.