Add CommandResult extensions for HasOption() and HasArgument()

[perlun]

This adds extension methods similar in spirit to the ones already present in ParseResultExtensions. I added these to my local project and decided to upstream them in the hope that they can be useful to others also. 🙏

The new methods don't have any tests unfortunately, but it could probably easily be added. Feel free to suggest an existing test class/method where tests for these would fit in well and I'll happily add one or more tests for them.

[perlun]

@jonsequitur - don't know the formal process here, but would be good to get this reviewed at some point if possible. 😃

[jonsequitur]

No complicated formal process, other than we'd want tests.

But that said, I'm wondering if CommandResult.HasOption() communicates this one particular nuance: the search (i.e. the underlying CommandResult.FindResultFor(IOption)) is across the whole ParseResult. I would read commandResult.HasOption(someOption) to mean "does this command's result contain this option?" But in fact the option in question might not be specified on that command, but instead on a parent command. Maybe this is fine?

[Keboo]

I agree with Jon. I think some of that might be cleared up by a slight name change. To try and communicate the intent with how it is work (a convenience helper for FindResultFor), perhaps simply including the term "Result" in the name.
So HasOption would become something like HasOptionResult.

Thoughts?

[perlun]

@Keboo

Thoughts?

We could go with that approach. The names I've chosen so far are similar to System.CommandLine.Parsing.ParseResultExtensions.HasOption - the newly added System.CommandLine.Parsing.CommandResultExtensions.HasOption is more or less identical to that method. So if we'd change this to HasOptionResult, maybe we should change the existing ParseResultExtensions.HasOption method as well?

Anyway, I'll go ahead and try to fix some tests now, they need to be done either way. 😊

[perlun]

Tests added, feel free to give it a look @jonsequitur @Keboo.

As far as the suffixing is concerned, I'm not fully convinced yet. The current form reads quite well, as least in the test:

            CommandResult completeResult = result.CommandResult;

            completeResult.HasArgument(stringArgument).Should().BeFalse();

            CommandResult completeResult = result.CommandResult;

            completeResult.HasOption(positionOption).Should().BeTrue();

Compared to the pre-existing tests in e.g. OptionTests, I think the above blends in quite well.

            var option = new Option<string>("-x");

            var result = option.Parse("");
            result.HasOption(option)
                .Should()
                .BeFalse();

[jonsequitur]

The distinction between the two that I'm pointing out (and yeah, I know I'm being very nitpicky) is that in the existing ParseResult.HasOption, the option's result is always a child of the ParseResult, while in the proposed CommandResult.HasOption, the option result is found via a lookup to the ParseResult. The option in question might not be a child of CommandResult.Command. In many cases it might be a child of a different command. When a subcommand is parsed, the ParseResult includes CommandResult instances for the subcommand and each of its ancestors, and can include options defined on those ancestors.

[perlun]

The option in question might not be a child of CommandResult.Command. In many cases it might be a child of a different command. When a subcommand is parsed, the ParseResult includes CommandResult instances for the subcommand and each of its ancestors, and can include options defined on those ancestors.

Ahh, thanks for making this a bit clearer. 👍 (I'm honestly quite a novice in this code base.)

I wonder if something like HasOptionRecursively and HasArgumentRecursively would do. It's a bit long and terrible, but we somehow want to make the point come through that the tree is being traversed in its entirety... I can probably also live with HasOptionResult and HasArgumentResult if both of you (@jonsequitur and @Keboo) prefer it this way.

Naming is indeed hard. 😅

[perlun]

I wonder if something like HasOptionRecursively and HasArgumentRecursively would do. It's a bit long and terrible, but we somehow want to make the point come through that the tree is being traversed in its entirety... I can probably also live with HasOptionResult and HasArgumentResult if both of you (@jonsequitur and @Keboo) prefer it this way.

Any feedback on these options (pun not intended 🙈) would be nice, I can try to get this finalized once we settle on the naming.

[jonsequitur]

@perlun I've proposed naming for a few new methods for related functionality here: #1127 (comment). Thoughts?

[perlun]

@perlun I've proposed naming for a few new methods for related functionality here: #1127 (comment). Thoughts?

@jonsequitur Hmm, interesting. So maybe something like CommandResult.HasOptionResultByName() and CommandResult.HasOptionResultByName() would be in line with those then. 🤔 Doesn't feel incredibly smooth perhaps, but maybe it's less confusing than the original names I suggested here at least.

[jonsequitur]

The key difference in behavior is whether the lookup encompasses the entire ParseResult. For those cases where it does, we've been using the verb "find" rather than "get" or "has". I'm not sure how to capture that nuance with "has".

It's possible we're better off just recommending the pattern commandResult.FindResultFor(argument) is { } rather than exposing a convenience bool-returning method here, since I would think that in the majority of cases, people will also want the corresponding ArgumentResult so they can get its value. The bool-returning method seems to guide people down a path where they'll make two invocations.

Out of curiosity, can you say more about your use cases for these new methods?

[perlun]

It's possible we're better off just recommending the pattern commandResult.FindResultFor(argument) is { } rather than exposing a convenience bool-returning method here, since I would think that in the majority of cases, people will also want the corresponding ArgumentResult so they can get its value. The bool-returning method seems to guide people down a path where they'll make two invocations.

Yeah, maybe you're right that it would be a bad fit for the "generic" nature that this library want to support. I think HasOption would perhaps be fine but HasArgument is the more questionable one. (If you like, I could rephrase this PR to only include HasOption.)

Out of curiosity, can you say more about your use cases for these new methods?

Sure. It's basically something like this (simplified example, the real-world code has more options etc).

var rootCommand = new RootCommand
{
    // Define Description, Handler etc
}

var evalOption = new Option<string>("-e", "Executes a single-line script") { AllowMultipleArgumentsPerToken = false, ArgumentHelpName = "script" };
var printOption = new Option<string>("-p", "Parse a single-line script and output a human-readable version of the AST") { ArgumentHelpName = "script" };

rootCommand.AddOption(evalOption);
rootCommand.AddOption(printOption);

var scriptNameArgument = new Argument<string>
{
    Name = "script-name",
    Arity = ArgumentArity.ZeroOrOne,
};

rootCommand.AddArgument(scriptNameArgument);

rootCommand.AddValidator(result =>
{
    if (result.HasOption(evalOption) && result.HasOption(printOption))
    {
        return "Error: the -e and -p options are mutually exclusive";
    }

    if (result.HasOption(evalOption) && result.HasArgument(scriptNameArgument))
    {
        return "Error: the -e option cannot be combined with the <script-name> argument";
    }

    return null;
});

[perlun]

This has been hanging for too long, and it's unlikely that it'll get merged as-is. I'll close it now, and instead try to figure out where IOption and IArgument went in the process of upgrading my app from 2.0.0-beta1.21216.1 to 2.0.0-beta4.22272.1... 😁

(Side note, but it would be incredibly nice with some form of release notes/changelog where a list of breaking changes like this can be tracked. I looked but couldn't find anything to the like; if it exists, please point me to it. 👍)

Edit: https://github.com/dotnet/command-line-api/issues?q=label%3A%22Breaking+Change%22+milestone%3A%222.0+GA%22+ actually has some of this. #1537 is also useful. 🙏

[perlun]

I'll close it now, and instead try to figure out where IOption and IArgument went

Ah, #1538 has me covered. 🎉