option without value can prompt or use default

[amy-lei]

Resolves #549
Resolves #736
Resolves #764
Resolves #921
Resolves #1015

Add prompt_required attribute to Option so that when prompt_required=False, the user will only be prompted for an input if the option is specified without a value. The attribute defaults to True to maintain the current behavior of option prompts.

is_flag=False now means that providing the value to the option is optional. If the flag is provided without the value, it can be prompted for or use flag_value.

[davidism]

I'm wondering if it's appropriate to have prompt get triggered from the parser, it seems like the parser should only be concerned with the command line, and prompting should happen during later processing.

[amy-lei]

I'm wondering if it's appropriate to have prompt get triggered from the parser, it seems like the parser should only be concerned with the command line, and prompting should happen during later processing.

I moved it into full_process_value since that's where it originally prompts for a value. But to do so, I had to add an optional param to let it know there was a flag present

[davidism]

I think there's another issue about options where the value is optional, maybe this solution will help with that too.

[davidism]

I was thinking of #549.

[amy-lei]

I was thinking of #549.

Just found it and was about to comment it too haha. Should I try to fit that into this PR too? Edit: actually, probably should be separate.

[davidism]

It seems like it should be a natural extension of this, since we now track if only the flag was passed. If it's only a flag, and there is a default, use the default instead of prompting. I don't think that should collide with any existing combination of settings, but that should be checked, along with how on/off flags currently work. It can be a separate PR.

[amy-lei]

@davidism I've added the ability to use options with optional args via the _flag_needs_value attribute we talked about. I currently have it so that setting prompt_required=False implies _flag_needs_value=True, and this simplifies the checks in the parser.

However, this did not get rid of the additional parameter we added to full_process_value because the only way to distinguish between the value being None due to the parser setting it that way and it being None due to the user not passing it in all can't be made without the opts dict.

An idea in mind is to instead of having prompt_required=False imply _flag_needs_value=True in the constructor, maybe we could set it to True in the parser? And then set it back after we've prompted? This way we wouldn't need the additional parameter anymore. But I'm also not too fond about this since we're changing attribute of the Option instance midway.

[davidism]

Instead of the parser producing None, it could produce a token that processing later on can check for.

# define the sentinel value at the top of the module
flag_needs_value = object()

# return it when parsing
def match(...):
    return _flag_needs_value

# check it when processing
def process(...):
    if value is _flag_needs_value:
        ...
    elif value is None:
        ...

[davidism]

I updated the PR description with the issues related to using a value instead of prompting. Also closed some existing PRs that will be handled by this. Let's make sure any tests they had are reflected here if applicable.

[amy-lei]

Sounds good 👍 I'll be sure to add those tests and update the docs.

Some clarifying questions:

1. If the user sets is_flag=False but does not pass in a flag_value, should an error be raised during initialization? during parsing? Or should it be fine to just let the value default to something like None instead?
2. From the description, does this mean that in order to allow the user to prompt when no value is passed, is_flag=False must be explicitly passed too? Is is just passing prompt_required=False sufficient?

[davidism]

1. For now it's better to require that flag_value be set. While we could always remove that restriction later, it would be harder to add it in.
2. For this, I think if is_flag is left as None, prompt_required=False can set it to False.

[amy-lei]

I've used the sentinel value idea, although was not too sure about the name or if it was appropriate to name it the same name as the attribute.

I also noticed #1230 enforced that the option cannot be multiple or have nargs!=1. I'm wondering if that should be the case here too or if it should be allowed instead. I did not add tests for it nor explicitly mention it in the docs yet.

[davidism]

Will get to this after #1649. They're all related, might affect my answer to the question about multiple and nargs.

Don't worry about rebasing in the mean time, I'll address that after the other things are merged.

[brendanator]

You may have already addressed this but I was attempting to implement something similar to the color option from git diff:

       --color[=<when>]
           Show colored diff.  --color (i.e. without =<when>) is the same as
           --color=always.  <when> can be one of always, never, or auto. It can
           be changed by the color.ui and color.diff configuration settings.

The color option has a flag value of always and defaults to auto so it will be covered by this functionality.

The main difference I see is that the value must be separated from the option by =. I think this is probably required to avoid the option gobbling up a parameter. e.g. git diff --color path/to/file should not set the color option to path/to/file

The example in the docs don't seem to handle this.

[davidism]

Working on rebasing this after #1649.