spectreconsole / spectre.console

A .NET library that makes it easier to create beautiful console applications.
https://spectreconsole.net
MIT License
9.1k stars 465 forks source link

Allow empty arguments when using double dashed args #594

Open Nisha2015 opened 2 years ago

Nisha2015 commented 2 years ago

Information

Describe the bug The issue was originally raised in Cake repository which is using Spectre.console.cli to parse the command line arguments.

A pattern that we've seen is to have use environment variables as values for Cake script arguments. e.g.

dotnet cake --argName=%ARG_NAME_VALUE% If the %ARG_NAME_VALUE% environment value exists and has a value, everything works as expected.

However, if the environment variable does not exist, or its value is empty, the user receives an error from the Cake runner: Expected an option value.

The current workaround is to use (space) instead of = if the value can be empty/null. e.g.

dotnet cake --argName %ARG_NAME_VALUE%

After investigating the, it is found that the issue is coming from Spectre.Console.Cli in ScanOptions method of CommandTreeTokenizer class. Below is the code throwing the exception, this is coming as the value of environment variable is empty or null, so the if condition i.e. if (!reader.TryPeek(out _)) is being satisfied and exception is thrown.

 if (reader.TryPeek(out character))
{
    // Encountered a separator?
    if (character == '=' || character == ':')
    {
        reader.Read(); // Consume
        context.AddRemaining(character);

        if (!reader.TryPeek(out _))
        {
            var token = new CommandTreeToken(CommandTreeToken.Kind.String, reader.Position, "=", "=");
            throw CommandParseException.OptionValueWasExpected(reader.Original, token);
        }

        result.Add(ScanString(context, reader));
    }
}

To Reproduce Run a cake script with an argument having an empty or null value i.e. dotnet cake --argName %ARG_NAME_VALUE%

Expected behavior User should be able to pass environment variables which has empty or null value.

Additional context Please look at https://github.com/cake-build/cake/issues/3277 for more information.


Please upvote :+1: this issue if you are interested in it.

Nisha2015 commented 2 years ago

We could just remove the line throwing CommandParseException and it will allow users to pass empty/null arguments in the command line. Consumer of the library would check if a value of an argument is required and will fail if that's the case.

I did test it and all the test cases pass except 4 in CommandAppTests.Parsing.cs which are checking the exception text.

I could fix those and raise a PR.

Please let me know your thoughts on the same. Thanks.

patriksvensson commented 2 years ago

@Nisha2015 If I've defined the following setting and passed an empty value, I expect it to fail since MyArg is required.

[CommandOption("--myarg <VALUE>")]
public string MyArg { get; set; }

To work around this, you need to use the FlagValue and set MyArg to optional.

[CommandOption("--myarg [VALUE]")]
public FlagValue<string> MyArg { get; set; }
tapika commented 2 years ago

Please note that [CommandOption("--myarg [VALUE]")] => [VALUE] is important as well, will not without it.

FrankRay78 commented 5 months ago

If I've defined the following setting and passed an empty value, I expect it to fail since MyArg is required. To work around this, you need to use the FlagValue and set MyArg to optional.

Hello @Nisha2015, did Patrik's explanation above fix this issue for you?