Support choices provided via the complete attribute

[mauvilsa]

I am interested in implementing #68. It is my first time looking at the shtab source code so do need some guidance. This pull request only implements the option to do like

# for type=Optional[bool]
action.complete = shtab.custom_choices("false", "true", "null")

Later I might also want to implement support the following, which could be relevant for discussing how to change the code now and later on.

# for type=Optional[Path_fr]
action.complete = [shtab.FILE, shtab.custom_choices("null")]

For the moment I have only implemented the support in optional actions. I will wait for some feedback before doing it for positionals.

[mauvilsa]

@casperdcl could you please take a look at this?

[mauvilsa]

action = parser.add_argument(..., choices=["false", "true", "null"])

instead of action.complete = ["false", "true", "null"]?

See the beginning of #68 for the motivation: "many shtab features would work out-of-the-box, avoiding the need for them to implement boilerplate". Making the users provide choices would be boilerplate. Take for instance a more complex case:

parser.add_argument("--options", type=Optional[Union[EnumA, EnumB, bool]])

Should users need to manually create an array of possible inputs and set choices? This would be duplication since that information is already in the type. The objective is that users don't need to set it. jsonargparse internally can take care of this.

Also I think there could be some undesired side effects. Setting argument choices restricts the input to those values. For example in the case bool. The command line completion would complete "true" or "false" (strings), but the parsed allowed values are True, False (not strings).

[simaoafonso-pwt]

Should users need to manually create an array of possible inputs and set choices? This would be duplication since that information is already in the type. The objective is that users don't need to set it. jsonargparse internally can take care of this.

Note that shtab is usable with simple argparse. Is this feature only available on jsonargparse?

If there's a boolean option, there are no choices to be selected, doesn't make much sense to combine those. The argparse input are always strings.

With type there are technically no limits to the valid strings, so if you want custom choices, it needs to be provided manually. I agree that with union types (or bool), for some types it's redundant, but not for all. How do you custom complete type=int for example?

[mauvilsa]

Note that shtab is usable with simple argparse.

Certainly. This feature does not limit anything related to argparse.

Is this feature only available on jsonargparse?

The motivation comes from jsonargparse. But this does not mean that it can't be used in other cases, be it argparse or other extensions of argparse. I haven't though of other real use cases. One I came up with right now for argparse could be: someone wants an argument with string type (or untyped) and from the command line provide completion of some prefixes. After the prefix can be any string, i.e.:

action = parser.add_argument("--arg", type=str)
action.complete = shtab.custom_choices("prefix1_", "prefix2_", "prefix3_")

Could be argued that there should be a specific completer for prefixes, so not applicable to custom_choices. This is just what came to mind this moment. Independent from this, I don't see any downsides if initially shtab.custom_choices was only used by jsonargparse.

If there's a boolean option, there are no choices to be selected, doesn't make much sense to combine those. The argparse input are always strings.

Not sure what you mean. argparse does not support type=bool. Anyway, if it doesn't make sense for an argument, then shtab.custom_choices should not be used.

With type there are technically no limits to the valid strings, so if you want custom choices, it needs to be provided manually. I agree that with union types (or bool), for some types it's redundant, but not for all. How do you custom complete type=int for example?

shtab.custom_choices is only intended for cases in which there is a limited list of possibilities. For types like int, then shtab.custom_choices would not be used.

Note that there is a different proposal for other kinds of types #70.

[simaoafonso-pwt]

Thanks for the feedback.

shtab.custom_choices is only intended for cases in which there is a limited list of possibilities.

That's I was trying to get at, as @casperdcl mentioned above.

action = parser.add_argument(..., choices=["false", "true", "null"])

In this common situation, why not use the argparse choices argument already? Why repeat this in a special argument?

I agree that the special argument custom_choices can be implemented, but default to using the choices parameter too, so that using choices on a regular argparse parameter works outside the box.

[casperdcl]

Hmm I think I have a meta-gripe. IIUC, this PR solves 2 different problems:

1. ArgumentParser.choices is not auto-populated based on ArgumentParser.type
2. hacky work-around for shtab to kinda use ArgumentParser.type (end user still needs to manually specify both type as well as .complete, which is not DRY)

I'd prefer a different approach:

1. fill in ArgumentParser.choices based on ArgumentParser.type (might not even be a shtab problem)
2. get shtab to use ArgumentParser.choices (already implemented)

WDYT?

[codecov-commenter]

Codecov Report

Patch coverage: 88.23% and project coverage change: +0.42 :tada:

Comparison is base (ed933a6) 91.06% compared to head (8762a89) 91.48%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #136 +/- ## ========================================== + Coverage 91.06% 91.48% +0.42% ========================================== Files 3 3 Lines 347 376 +29 ========================================== + Hits 316 344 +28 - Misses 31 32 +1 ``` | [Impacted Files](https://app.codecov.io/gh/iterative/shtab/pull/136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative) | Coverage Δ | | |---|---|---| | [shtab/\_\_init\_\_.py](https://app.codecov.io/gh/iterative/shtab/pull/136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative#diff-c2h0YWIvX19pbml0X18ucHk=) | `93.63% <88.23%> (+0.28%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[mauvilsa]

In this common situation, why not use the argparse choices argument already? Why repeat this in a special argument?

1. ArgumentParser.choices is not auto-populated based on ArgumentParser.type

As I explained before, custom_choices is for cases in which the completions are supposed to be different to the argument's choices. One example is type=Optional[bool]:

• the choices being [True, False, None] (not strings, these are allowed values in namespace after parsing)
• and the completions ["true", "false", "yes", "no", "null"] (must be strings)

2. hacky work-around for shtab to kinda use ArgumentParser.type (end user still needs to manually specify both type as well as .complete, which is not DRY)

The main motivation is jsonargparse, see the docs. The objective is that jsonargparse takes care of automatically setting the custom_choices. There is no need for users to introduce duplication. Quite the contrary, one of the purposes is reducing boilerplate. Example:

parser.add_argument("--bool", type=Optional[bool])  # custom_choices are automatically set
                                                    # no need to set choices either

A second example with even less duplication is adding arguments from a function signature. The function already has type hints, so there is no reason to duplicate these to create a parser:

parser.add_function_arguments(func_many_params)  # adds many arguments, custom_choices automatic if applicable

For this to be possible to implement in jsonargparse, there is a need to have in shtab a way to configure such custom choices. Not intended to be a hack, but an official way of doing this.

Apart from jsonargparse there could be other use cases. Examples:

• Alternative extensions to argparse that also automatically add custom_choices
• People who for some reason explicitly want to set the completion choices to be different to the argument choices

Just for reference. shtab already works with jsonargparse and people have been using it for some time, e.g. https://github.com/omni-us/jsonargparse/issues/108#issuecomment-985312470. I would encourage you to try it out for better understanding of the use case. You could even enable argcomplete (see docs) to have an actual tab completion working.

If this is not enough motivation to justify adding a feature in shtab, then it is fine to decline this pull request. I am also open for suggestions on an alternative. However, ArgumentParser.choices does not seem to meet the criteria needed in jsonargparse.

[mauvilsa]

I am also open for suggestions on an alternative. However, ArgumentParser.choices does not seem to meet the criteria needed in jsonargparse.

I think I have come up with a viable alternative using complete. It has the drawback that if someone uses add_argument_to then the completion script would not include the choices. But can be good enough. Closing this.

[casperdcl]

Oh do you mean something like this?

class Custom:
    def __init__(self):
        self.preamble_bash = ""
    def choice(*choices):
        assert all(isinstance(c, str) for c in choices)
        choices_str = "('" + "' '".join(choices) + "')"
        func_bash = f"_{hash(choices_str)}"
        self.preamble_bash += f"""\
{func_bash}_choices={choices_str}
{func_bash}(){
  compgen -W "${{{func_bash}_choices[*]}}" -- $1
}
"""
        return {"bash": func_bash, "zsh": choices_str, "tcsh": choices_str}

With the use case:

custom = Custom()
parser.add_argument("--bool", type=Optional[bool]).complete = custom.choice("yes", "no", "null")
parser.add_argument(...).complete = custom.choice(...)
shtab.add_argument_to(parser, preamble={"bash": custom.preamble_bash})

[mauvilsa]

No, what I had in mind is to have a wrapper to PrintCompletionAction. If shtab already supports ArgumentParser.choices then I can populate it. However, populating it makes the parser useless because trying to parse would fail. So the though was a custom action that first populates the choices and the does the print. Since the parser is invalid only when the user requests to print, the issue is avoided. jsonargparse could take care of automatically adding a print argument with the wrapper action. The drawback is someone manually using add_argument_to which would use the original action.

Regarding your idea, what I don't particularly like is that every developer would have to implement that class in their code. Common completion use cases should become functions/classes part of shtab and not need to do a preamble. An example being #69.

For the custom choices, if the only likely potential user is jsonargparse and there is an alternative, then it can be considered an uncommon use case. No good reason to add it to shtab. Which is why I closed this.

[casperdcl]

every developer would have to implement that

That's true for both PrintCompletionAction and Custom.choice methods.

if the only likely potential user is jsonargparse and there is a [work-around] then it can be considered an uncommon use case

Ok, makes sense. Happy to reopen/reconsider if there are more requests/use cases :)