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] Test has incorrect errors for `-x 1 2 3 4` #2392

Open KathleenDollard opened 2 months ago

KathleenDollard commented 2 months ago

The test I used to confirm unexpected behavior

The test is:

      [Fact]
      public void When_option_arguments_are_greater_than_maximum_arity_then_an_error_is_returned()
      {
          var command = new CliCommand("the-command")
                  {
                      new CliOption<int[]>("-x") { Arity = new ArgumentArity(2, 3)}
                  };

          var parseResult = CliParser.Parse(command, "-x 1 2 3 4");
          parseResult.Errors
                     .Select(e => e.Message)
                     .Should()
                     .Contain(LocalizationResources.UnrecognizedCommandOrArgument("4"));
      }

I modified the test only to get an interim variable for parseResult,

When I do this, the error list is

image

I encountered this because for Powderhouse I encountered issues with options with collection types and I believe I am using the same or funcionally equivalent code - and this test passes. In exploring why my new tests were failing, I realized this test does not appear to be succeeding at what the CLI and the commandline request.

I believe this test should result in a single parse error - an unexpected argument only on the 4.

Keboo commented 2 months ago

I think this may be a bug (even on main).

First, the test input is bad. The successful test a couple up shows how an option's argument with a minimum arity greater than 1 should be tested. Specifically the test input should be: command.Parse("-x 1 -x 2 -x 3 -x 4").

Now with that, said, even once that is corrected that will only solve your first three "Unrecognized command or argument" errors from your screenshot. You are then left with two "Option '-x' expects a single argument but 4 were provided." errors. I would propose both the error message is incorrect, as well as the fact that there are two errors (I agree there really should only be one). This is not something new with powerhouse, this is existing behavior on main.

elgonzo commented 2 months ago

For this test to succeed, it would be required to set the option's AllowMultipleArgumentsPerToken property to true: https://github.com/dotnet/command-line-api/blob/4457da40255a9dd5cc411addd7a6d00360b03898/src/System.CommandLine/CliOption.cs#L85-L99 This is of course under the assumption that you want to keep this property in powderhouse,.

(Commit and discussion related to making this disabled by default during past SCL development: https://github.com/dotnet/command-line-api/issues/1199, https://github.com/dotnet/command-line-api/issues/1162)