spectre.console

spectre.console copied to clipboard

Information

• OS: macOS
• Version: 0.43.0
• Terminal: macOS Term

Describe the bug A combination of a boolean flag and a positional argument leads the the error Flags cannot be assigned a value., which is wrong in my opinion.

To Reproduce Use the following settings:

// ...
    [Description("Input files or folders")]
    [CommandArgument(0, "[input]")]
    public string[] Input { get; set; } = Array.Empty<string>();

    [CommandOption("--dry-run")] public bool DryRun { get; init; } = false;
// ...

And call the app like this:

myapp --dry-run "my-input-folder/"

Expected behavior As --dry-run is a flag, I would expect spectre.console to handle the fact, taht there is a positional argument in the settings and parse this correctly. In my opinion this should work without error messages.

Workaround Putting the flag at the end works like expected:

myapp "my-input-folder/" --dry-run

Added a small cli-tester to my open source project to demonstrate the issue: https://github.com/sandreas/tone/tree/main/cli-tester

# error, although --dry-run is a boolean flag without any value
cli-tester test --dry-run input.mp3

Error: Flags cannot be assigned a value.

       test --dry-run input.mp3
            ^^^^^^^^^ Can't assign value


# works like expected (just put the flag at the end)
cli-tester test input.mp3  --dry-run

Since this is an issue with UX, I think this is a serious one. I would really appreciate ANY feedback...?! Maybe I'm doing something wrong?

@patriksvensson Sorry to adress you directly but I just wanted to ask, if this is the right place to report these kind of issues or am I doing something wrong?

This seems a duplicate of https://github.com/spectreconsole/spectre.console/issues/193 @patriksvensson

@FrankRay78 You are right, sorry, I did not find this. If you are looking for a reason, why this is not working as expected, you should take a look this.

the tokenizer should not silently ignore valid chars or make invalid assumptions about the context (e.g. assume that every args value starting with - is automatically an option)... this is something the lexer, parser or context should be aware of...

I think the tokenizer treats every argument the same way and is not aware of it's context - which is a real problem when using flags (boolean), because the tokenzier / lexer only knows, that the next argument cannot be a value, if it is aware of the type of already parsed arguments...

In my opinion all these issues are related to each other. Maybe the right way to do this would be:

• Take the next argument
• Check if the argument matches a setting property
  • If yes, check if the argument needs a value (yes, if it is a setting, no if it is a flag)
    • If yes, take the next argument value as is (without parsing anything or assuming - is a setting argument or flag)
    • If no, set the flag and start over
  • If no, check if the argument already contains a value (with =)
    • If yes, parse and start over
    • If no, it is a parsing error

If I am right, currently the parser treats every argument the same without looking at the context.

Hi @sandreas, it's interesting your comment above, and also your comment here https://github.com/spectreconsole/spectre.console/issues/959

I think we are saying the same thing but I would appreciate if you could cast your eyes over my most recent comment here: https://github.com/spectreconsole/spectre.console/issues/193

Also, the CommandTreeParser is a surprisingly long class with most of the methods marked as private (ie. small number of entry points combined with complex internal logic), which as you indicated elsewhere, doesn't bode well for test coverage.

I think we are saying the same thing but I would appreciate if you could cast your eyes over my most recent comment here: https://github.com/spectreconsole/spectre.console/issues/193

Thank you for your quick response. I did read your most recent comment - maybe I did not fully understand, because I'm not too much into the spectre.console code, but I think what you describe is only part of the problem...

The command line parser does indeed handle boolean options (flags) the wrong way under specific circumstances, but NOT ONLY flags. Afaik it does not distinguish between FLAGS, OPTIONS, VALUES, ARGUMENTS and OPTION=VALUE combinations, depending on the context it currently has. This seems to cause the flag issue, but also to the -, " and <whitespace> cannot be used as first char of a VALUE (because it goes through a parser, although it shouldn't and should be kept as is).

Example - let's say you have the following Options-Class:

public class TestCommandSettings: CommandSettings
{
    [CommandOption("--dry-run")] public bool? DryRun { get; init; } = false;
    
    [CommandOption("--order-by")] public string? OrderBy { get; set; }
    
    [Description("Input files or folders")]
    [CommandArgument(0, "[input]")]
    public string[] Input { get; set; } = Array.Empty<string>();
}

# 1. FLAG, next token MUST be OPTION, OPTION=VALUE, ARGUMENT or end of tokens
# --dry-run
cli-tester --dry-run

# 2. OPTION, next token MUST be VALUE (e.g. for OrderBy-Option)
# --order-by 
cli-tester --order-by "size"

# 3. OPTION=VALUE, next token MUST be OPTION, OPTION=VALUE, ARGUMENT or end of tokens
# --order-by="size"
cli-tester --order-by="size"

# 4. ARGUMENT, next token MUST be OPTION, OPTION=VALUE, ARGUMENT or end of tokens
# "my-directory/"
cli-tester "my-directory/"

# combined example
cli-tester --order-by "-size" --dry-run "my-directory"

So in my opinion the parser needs to be redesigned to:

• Not look for VALUE after FLAG, OPTION=VALUE or ARGUMENT
• Force having a VALUE after every valid OPTION
• Never "parse" any VALUE or ARGUMENT (just keep as is)

The only real "parsing" needs to be done, when there is OPTION=VALUE, in every other case it just needs a check, if it is a valid FLAG or OPTION followed by an untouched VALUE or it can be an ARGUMENT.

In short: Why "parse" --order-by and "-size" separately, when we already know, that --order-by is a perfect match in our OPTION reference and it must always be followed by a VALUE, that does not need to be parsed anyway.

I hope this was understandable... pretty long text for a pretty simple algorithm ;)

Excellent write up and example @sandreas. I plan to fix the other issue, which is one step forward and will let me learn the parser and improve code coverage. Then come back to this issue.

Some small additions / edge-cases:

• What happens if there single quotes enclosing double quotes with equal sign: "--meta-description='\"What?\"'"
• If there is multiple flags with short form --force|-f and --debug|-d, a common short form is -df to enable both flags
• If there is an option with short form --order-by|-o, the value can be set via -osize

Just a few of my other thoughts...

@sandreas -df is already supported.

-df is already supported.

Didn't know that. thx

@FrankRay78

Would it help, if I compose a set of edge case parameter sets (e.g. --option '"value" test') compile the following code as x64 for Windows, macOS and Linux and provide the output of these edge cases on all platforms?:

foreach (var arg in args)
{
    Console.WriteLine(arg);
}

This issue can be closed/marked as complete @patriksvensson, now that PR https://github.com/spectreconsole/spectre.console/pull/1048 has been successfully merged.