add support for required options

[jonsequitur]

Add support for making an option, or one option out of a group of options, required. While this can currently be done using a custom validator, it's awkward and not obvious.

Related: #381.

[tmds]

UX example using dotnet-counters from https://github.com/dotnet/diagnostics:

$ dotnet counters --help
Usage:
  dotnet-counters [options] [command]

Options:
  --version    Display version information

Commands:
  monitor <counter_list>    Start monitoring a .NET application
  list                      Display a list of counter names and descriptions, grouped by provider.
  list-processes            Display a list of dotnet processes that can be monitored.

$ dotnet counters monitor
ProcessId is required.

code:

        private static Command MonitorCommand() =>
            new Command(
                "monitor", 
                "Start monitoring a .NET application", 
                new Option[] { ProcessIdOption(), RefreshIntervalOption() },
                argument: CounterList(),
                handler: CommandHandler.Create<CancellationToken, List<string>, IConsole, int, int>(new CounterMonitor().Monitor));

        private static Option ProcessIdOption() =>
            new Option(
                new[] { "-p", "--process-id" }, 
                "The ID of the process that will be monitored.",
                new Argument<int> { Name = "pid" });

[darinclarkdev]

Until the api is decided this is what works for me.

cmdBuilder = new CommandLineBuilder(rootCommand)
				.UseRequiredOptions()
				.UseDefaults();

public class RequiredOption : Option {
    public RequiredOption(string alias, string description) : base(alias, description) => this.Description = $"[REQUIRED] {description}";
    public RequiredOption(string[] aliases, string description) : base(aliases, description) => this.Description = $"[REQUIRED] {description}";
}

public static class CommandLineBuilderExtensions {
    public static CommandLineBuilder UseRequiredOptions(this CommandLineBuilder value) {
        value.UseMiddleware(async (context, next) => {
	    bool errorfound = false;
            var options = context.ParseResult.CommandResult;
            foreach (Option item in options.Command.Children) {
                if (item is RequiredOption && !options.Children.Any(s => s.Name == item.Name)) {
                    context.Console.Out.WriteLine($"{item.Name} is a [REQUIRED] option\n");
                    errorfound = true;
                }
            }
            if (!errorfound)
                await next(context);
            else {
                var help = new HelpResult();
		help.Apply(context);
            }
        });
	return value;
    }
}

[guy-lud]

Doesn't ArgumentArity helps you ?

[darinclarkdev]

My understanding is Arity only helps if the option is used.

[guy-lud]

ArgumentArity allows you to use minimum level and max level so by using 1 and 1 forces you to use the option. Is that what you meant ?

[darinclarkdev]

I will check it out. No need to re-invent the wheel if that works.

[jonsequitur]

The argument arity describes the number of arguments that are valid for an option or command if that option or command is specified. If the option or command isn't specified, then its argument arity is immaterial.

There are some additional details here:

https://github.com/dotnet/command-line-api/wiki/How-To#argument-validation-and-binding

[darinclarkdev]

@jonsequitur thanks for clarifying argument arity.

[johncrim]

Right now, you can have required positional arguments (no parent Option), but not not required named arguments (Argument has a parent Option).

I think the combination of Argument.Arity.Min = 1 and no default value clearly indicates that the argument is required (works today for positional arguments). I don't think there's a need to add a separate .Required property, that would just add redundancy to the API. A developer can get the same behavior (optional named parameters) as today just by setting Arity.Min = 0 or setting a default value. What would Option.Required mean in combination with Arity.Min = 0 or an Argument.DefaultValue? I think it would add unnecessary complexity to the API.

[Alxandr]

I tried implementing this, but it breaks some tests.

Example:

        [Fact]
        public async Task Nullable_parameters_are_bound_to_null_when_option_is_not_specified()
        {
            var wasCalled = false;
            int? boundAge = default;

            var command = new Command("command")
            {
                new Option("--age")
                {
                    Argument = new Argument<int?>()
                }
            };
            command.Handler = CommandHandler.Create<int?>(age =>
            {
                wasCalled = true;
                boundAge = age;
            });

            await command.InvokeAsync("command", _console);

            wasCalled.Should().BeTrue();
            boundAge.Should().BeNull();
        }

Doing new Option("--age") { Argument = new Argument<int?>() } results in it now being a required argument. I guess the question is whether or not we expect this test to pass.

[giggio]

This is super important. The workarounds may work, but the help text is not changed. We need a full solution, this is a basic feature.

[darinclarkdev]

@giggio look at my solution
#560 (comment)

[giggio]

@darinclarkdev your solution is the one we are using right now, and it is a good workaround, but the docs from --help still don't show which parameters are required.

[darinclarkdev]

@giggio I have updated the RequiredOption constructor in my example. It now prepends [REQUIRED] to the description.

[Alxandr]

I would still like to know whether or not it's logical for the unit-test above to pass. I have a branch locally that mostly adds support for required options, but it obviously breaks the unit-test above, because it directly contradicts the requirement.

[jonsequitur]

@Alxandr I think it makes sense for the test above to pass. The reasoning would be specific to the type int? which, because it's nullable, should have an inferred minimum arity of 0.

Edit: Turns out I was wrong here. Sorry for the confusion. See below and here.

[jonsequitur]

I think the combination of Argument.Arity.Min = 1 and no default value clearly indicates that the argument is required (works today for positional arguments). I don't think there's a need to add a separate .Required property, that would just add redundancy to the API. A developer can get the same behavior (optional named parameters) as today just by setting Arity.Min = 0 or setting a default value. What would Option.Required mean in combination with Arity.Min = 0 or an Argument.DefaultValue? I think it would add unnecessary complexity to the API.

@johncrim Agreed. It might be that adding an extension method like Option.Required() could simply set the arity, making the capability more discoverable and intuitive without actually introducing a new concept.

@giggio The help output should currently reflect the arity requirements. If it doesn't it's clearly a bug.

[Alxandr]

Ah. I'm so used to nullable reference types by now that I've learned to overlook nullable value types to xD. You're right, I'll have to look into why it's failing.

[Alxandr]

@jonsequitur I went back to look at this, and there seems to be something weird going on. This is the age option from the unit test above when I debug it:

As you can clearly see, MinimumNumberOfValues is 1 (and HasDefaultValue is false if I scroll down), so there seems to be something wrong with how nullables are dealt with?

[jonsequitur]

I tried to clarify this in this comment.

[jonsequitur]

I think the combination of Argument.Arity.Min = 1 and no default value clearly indicates that the argument is required (works today for positional arguments). I don't think there's a need to add a separate .Required property, that would just add redundancy to the API. A developer can get the same behavior (optional named parameters) as today just by setting Arity.Min = 0 or setting a default value. What would Option.Required mean in combination with Arity.Min = 0 or an Argument.DefaultValue? I think it would add unnecessary complexity to the API.

@johncrim It turns out that there's an ambiguity here that I missed earlier, so I'd like to clarify my response.

To my understanding, the following statement is absolutely aligned with the design:

I think the combination of Argument.Arity.Min = 1 and no default value clearly indicates that the argument is required (works today for positional arguments).

But here's the potential ambiguity. What you're calling positional arguments are represented in the API as Command.Arguments. An option is a different thing. An option is not an argument; and option has arguments. Option.Argument.Arity describes how many arguments it can have, syntactically.

The reason we don't have a concept in the API right now for an option's arity is that we wanted the following to all be equivalent:

> myapp -x 1 -x 2 -x 3
> myapp -x 1 2  -x 3
> myapp -x 1 2 3

A developer can get the same behavior (optional named parameters) as today just by setting Arity.Min = 0 or setting a default value.

This is not quite the case. If for an option -x the minimum argument arity is 0, the following is valid:

myapp -x

The most common use case for arity 0 is, by far, binding to bool, i.e. this is a flag.

If option -x has a minimum argument arity of 1, the above would be invalid. So the default value doesn't affect the parsing of the option, it describes what value will be bound if the option is not specified.

What would Option.Required mean in combination with Arity.Min = 0 or an Argument.DefaultValue?

Option -x being required would mean the following would be invalid because -x was not provided:

> myapp

The above would also be valid if the argument to option -x had a default value.

My goal here is to clarify the current design, not to argue that it's ideal. This part of it can clearly be made more intuitive.

[Alxandr]

@jonsequitur Back to the drawing board then. Would you thing having Option.Required = true property is better? Or is there some other API you think better fits the problem?

[jonsequitur]

I'm not sure. I do think that a Required property is fairly intuitive, while argument arity (for options) is not. #674 might help in the sense that it makes Option.Argument.Arity less visible. Argument arity is both necessary from a syntax perspective and almost always unnecessary for what users of the API want to accomplish.

[darinclarkdev]

This removes the need for my workaround. I would have preferred the help to use the option name instead of the first alias but I will get over it.

[jonsequitur]

The option name doesn't include the prefix, so it's a little less helpful in practice.

[brandonschmidt]

Technically speaking, aliases don't have to include the prefix either. Both of these are valid and (semi-)functional, but they return different help texts.

var command = new RootCommand() { new Option(new[] { "-i", "--input" }, "Description") };

var command = new RootCommand() { new Option(new[] { "i", "input" }, "Description") };

Only the first one will actually work as expected, however. The 2nd alias doesn't work in the 2nd example above. This is because the code doesn't use the Prefixes variable consistently. (And the Prefixes variable doesn't support the concept of long and short names.)

[jonsequitur]

The 2nd alias doesn't work in the 2nd example above.

@brandonschmidt What specifically doesn't work?