tap.utils.get_argument_name() should choose a canonical argument name like ArgumentParser.add_argument() does

[Deltik]

Motivation

tap.tap.Tap.add_argument() restricts the *name_or_flags vararg differently from ArgumentParser.add_argument().

Consider this pure argparse example:

import argparse

if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("-t", "--with", "--to",
                        metavar="TARGET",
                        dest="target_name",
                        help="connect to this target",
                        )
    args = parser.parse_args()
    print(args)

argparse will choose the value of dest as the argument name if available. Otherwise, it will choose the first long argument name as the argument name.

$ python3 /tmp/scratch.py
Namespace(target_name=None)

$ python3 /tmp/scratch.py --help
usage: scratch.py [-h] [-t TARGET]

optional arguments:
  -h, --help            show this help message and exit
  -t TARGET, --target TARGET, --to TARGET
                        connect to this target

But Tap.add_argument() will refuse to do the same:

from tap import Tap


class ScratchParser(Tap):
    target_name: str  # connect to this target

    def configure(self) -> None:
        super().configure()
        self.add_argument("-t", "--target", "--to", dest="target_name", metavar="TARGET")


if __name__ == "__main__":
    args = ScratchParser().parse_args()
    print(args)

$ python3 /tmp/scratch_1.py --help
Traceback (most recent call last):
  File "/tmp/scratch_1.py", line 13, in <module>
    args = ScratchParser().parse_args()
  File "~/lib/python3.8/site-packages/tap/tap.py", line 103, in __init__
    self._configure()
  File "~/lib/python3.8/site-packages/tap/tap.py", line 309, in _configure
    self.configure()
  File "/tmp/scratch_1.py", line 9, in configure
    self.add_argument("--target-name", "-t", "--with", "--to", metavar="TARGET")
  File "~/lib/python3.8/site-packages/tap/tap.py", line 259, in add_argument
    variable = get_argument_name(*name_or_flags).replace('-', '_')
  File "~/lib/python3.8/site-packages/tap/utils.py", line 145, in get_argument_name
    raise ValueError(f'There should only be a single canonical name for argument {name_or_flags}!')
ValueError: There should only be a single canonical name for argument ['--target', '--to']!

I expected Typed Argument Parser to associate the added argument to the attribute ScratchParser.target_name because the keyword argument dest="target_name" was specified.

If dest="target_name" had not been specified, then I would have expected the canonical argument name to be target because that's what ArgumentParser.add_argument("-t", "--target", "--to", …) would have done.

And if it had been ArgumentParser.add_argument("-t", "-n", …) instead, the canonical argument name would have been t because that is the first name specified and there was no option that began with two hyphens to take precedence.

Proposed Solution

tap.utils.get_argument_name() should be rewritten to determine the canonical argument name based on this order of preference:

1. The value of the dest keyword argument, if it is a string / not None.
2. The first *name_or_flags that begins with "--", if any.
3. The first *name_or_flags that begins with "-".

Behaviors that should be unchanged but may be worth testing:

• If the argument is a positional argument (i.e. it does not begin with "-"), it must be the only item in *name_or_flags.
• It is an error to mix a positional argument (e.g. "target") and an option (e.g. "--target").

Alternatives

I have not been able to find a way to Tap.add_argument() two long-named option aliases. The alternative for this case would be to use plain argparse, but I'd lose the typing benefits offered by Typed Argument Parser.

Without ArgumentParser.add_argument(…, dest=…) support, the first long-named option would have stand in for dest. This is not ideal if I want to receive an primary option named with a reserved keyword like ArgumentParser.add_argument("--with", "--to", dest="target_name").

[martinjm97]

Great point. We'll work on this soon. Thank you so much for the thorough and extremely thoughtful write up.

[bland328]

I'm attempting to adapt an ArgumentParser-based project to use typed-argument-parser instead, but immediately ran into this same stopper.

Has any progress been made on this? Or is there current a workaround that would enable a construct like this to work?

self.add_argument('--dry', '--dryrun', '--dry-run', dest = 'dryrun', action = 'store_true'...

For the record, @Deltik's proposed solution above sounds right to me.