dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.35k stars 375 forks source link

[main] Issue with this pattern: `inner -x one -x two` #2388

Open KathleenDollard opened 2 months ago

KathleenDollard commented 2 months ago

My understanding of Posix rules for

inner -x one -x two

is that it should result in "two" for "-x" when the declaration is what is included in this test:

  public void Options_only_apply_to_the_nearest_command()
  {
      var outerOption = new CliOption<string>("-x");
      var innerOption = new CliOption<string>("-x");

      var outer = new CliCommand("outer")
                  {
                      new CliCommand("inner")
                      {
                          innerOption
                      },
                      outerOption
                  };

      var result = outer.Parse("outer inner -x one -x two");

      //result.Errors.Should().BeNullOrEmpty();
      //result.GetValue<string>(innerOption).Should().Be("two");
      result.RootCommandResult
            .GetResult(outerOption)
            .Should()
            .BeNull();
  }

I added the two commented out lines, and with either enabled, the test fails.

This is one of a handful of tests that are failing as I work on Powderhouse, and the behavior of System.CommandLine in these cases is not what I expected.

The biggest thing I need right now is whether this scenario should pass with the two commented out lines enabled.

@jonsequitur @keboo

KalleOlaviNiemitalo commented 2 months ago

I don't remember POSIX itself specifying any subcommands, or how options should be parsed with subcommands.

My hunch is that, in inner -x one -x two, both -x tokens should map to the option of the inner subcommand; and if that does not support repeated -x options, then it's a parse error. If the user wants -x one to map to the option of the root command, then the command line should be -x one inner -x two instead. That's how it works in Git.

But perhaps "Posix rules" refers to historic System.CommandLine behaviour with which you want to preserve compatibility.

Keboo commented 2 months ago

There are two things at play here. First is the arity of the option. In this case it is zero or one because it is CliOption<string>. The second thing is the CliOption.AllowMultipleArgumentsPerToken. There are some XML docs on that help explain it as well.

With AllowMultipleArgumentsPerToken set to false, the presence of multiple values for the innerOption then is expected to result in a parse error because it was defined to only accept zero or one, and not to overwrite with the latest.

So in that test above, to make it work as you expect you would do this:

[Fact]
public void Options_only_apply_to_the_nearest_command()
{
    var outerOption = new CliOption<string>("-x");
    var innerOption = new CliOption<string>("-x") { AllowMultipleArgumentsPerToken = true };

    var outer = new CliCommand("outer")
                {
                    new CliCommand("inner")
                    {
                        innerOption
                    },
                    outerOption
                };

    var result = outer.Parse("outer inner -x one -x two");

    result.Errors.Should().BeNullOrEmpty();
    result.GetValue<string>(innerOption).Should().Be("two");
    result.RootCommandResult
            .GetResult(outerOption)
            .Should()
            .BeNull();
}

Now there is an argument to be made that the default of false for AllowMultipleArgumentsPerToken is wrong and it should default to true, but that is a slightly different discussion.

elgonzo commented 2 months ago

AllowMultipleArgumentsPerToken is unrelated to this. As the linked XML doc explains and demonstrates, this parameter allows or disallows a syntax where a single occurrence of a an option token can be followed by multiple option argument tokens, like the example given in the XML doc --opt 1 2 3.

But there are no multiple argument tokens per option token in inner -x one -x two. Every one of the -x option tokens has only one argument token, so setting AllowMultipleArgumentsPerToken to true would have no bearing on this particular situation, according to its XML doc.