Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
KathleenDollard opened this issue Apr 20, 2024 · 3 comments
Open

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

KathleenDollard opened this issue Apr 20, 2024 · 3 comments

Comments

@KathleenDollard
Copy link
Contributor

KathleenDollard commented Apr 20, 2024

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
Copy link

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
Copy link
Member

Keboo commented Apr 20, 2024

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
Copy link
Contributor

elgonzo commented Apr 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants