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

Open
KathleenDollard opened this issue Apr 23, 2024 · 2 comments
Open

[main] Test has incorrect errors for -x 1 2 3 4 #2392

KathleenDollard opened this issue Apr 23, 2024 · 2 comments

Comments

@KathleenDollard
Copy link
Contributor

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

Keboo commented Apr 23, 2024

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

elgonzo commented Apr 23, 2024

For this test to succeed, it would be required to set the option's AllowMultipleArgumentsPerToken property to true:

// TODO: what does this even mean?
/// <summary>
/// Gets a value that indicates whether multiple argument tokens are allowed for each option identifier token.
/// </summary>
/// <example>
/// If set to <see langword="true"/>, the following command line is valid for passing multiple arguments:
/// <code>
/// > --opt 1 2 3
/// </code>
/// The following is equivalent and is always valid:
/// <code>
/// > --opt 1 --opt 2 --opt 3
/// </code>
/// </example>
public bool AllowMultipleArgumentsPerToken { get; set; }
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: #1199, #1162)

Keboo added a commit to Keboo/command-line-api that referenced this issue Apr 25, 2024
There were two problems addressed here.
First, the call to Diagram was causing the second error to be added to the ParseResult.Errors collection. The diagram call, was being invoked by the debugger as part of the ParseResult.ToString() method. Causing the number of errors to change when inspecting a parse result.

The second issue was the error message text. It was implying that the option expected a single value, when its arity allowed for more.
Keboo added a commit to Keboo/command-line-api that referenced this issue May 4, 2024
Fixes dotnet#2392.
This is a port from the fix on main. However due to additional work that is currently being done on Powerderhouse it is forcing the creation of the ArgumentConversionResult as part of parsing. This is due to the invocation SymbolResultTree.GetValueResultDictionary() which forces the evaluation of all ArgumentConversionResults. This is adding an additional test to make sure that the behavior is not re-introduced. This also fixes a few NRT issues and a comment typo.
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

3 participants