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

CLI actions prototype #2065

Closed
wants to merge 4 commits into from

Conversation

jonsequitur
Copy link
Contributor

@jonsequitur jonsequitur commented Feb 23, 2023

These changes are a prototype for #2071.

As a next step, custom actions types should be able to be either generated or bound to command line values using source generators. More investigation is needed there.

Feedback and suggestions are welcome.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single

Exposing an Action or a Symbol from ParseResult

public class ParseResult
{
    public CliAction Action { get; }
    public Symbol Symbol { get; }
}

has one important limitation: it assumes that only one Symbol provides an action that needs to be executed.

While implementing #2063 I've realised that it's not enough for directives. Example: [env:key=value] wants to just set the env var(s) and run the parsed command after that.
But some directives don't want to continue the execution. Example: [parse] just wants to print a diagram and quit.
Moreover, the directives can be combined. We may want to set env vars, current culture and even implement something like [stopwatch] and then just run the command.

After thinking about it for a while I believe that we already have all that is needed in order to remove middleware.

For the example provided by @KathleenDollard in #2048 we can just use ParseResult.FindResultFor(Symbol) which returns null if given symbol was not parsed. Since we care about the active symbols only, it's enough to identify every scenario. But we would need to expose HelpOption and VersionOption types, which IMO is perfectly fine.

public static int Run(Command root, string[] args, CommandLineConfiguration configuration = default)
{
    ParseResult parseResult = root.Parse(args, configuration);

    if (parseResult.Errors.Count != 0)
    {
        return DisplayValidationErrors(configuration, parseResult);
    }
    else if (parseResult.FindResultFor(new HelpOption()) is not null)
    {
        DisplayHelp(configuration, parseResult);
    }
    else if (parseResult.FindResultFor(new VersionOption()) is not null)
    {
        DisplayVersion();
    }
    else if (parseResult.FindResultFor(new ParseDirective()) is not null)
    {
        DiagramInput(parseResult);
    }
    else if (parseResult.FindResultFor(new SuggestDirective()) is not null)
    {
        SuggestResult(parseResult); // this is for tab completion
    }
    else
    {
        InvokeCommand(configuration, parseResult, invocationResult),
    }
}

It's not 100% intuitive yet, as it works because these types override Equals and GetHashCode and report equality for the same type, not for the same instance:

public override bool Equals(object? obj) => obj is HelpOption;
public override int GetHashCode() => typeof(HelpOption).GetHashCode();

So we could just introduce:

public ParseResult
{
    public SymbolResult? FindResultFor<T>() where T : Symbol
}

But the implementation would not be 100% perf optimal (we would need to go over all parsed symbols and cast until we find the first instance).

So we could introduce a name-based overload which would not require to make HelpOption and VersionOption public nor store references to symbols:

public ParseResult
{
    public SymbolResult? FindResultFor(string name)
}

And have sth like this:

public static int Run(Command root, string[] args, CommandLineConfiguration configuration = default)
{
    ParseResult parseResult = root.Parse(args, configuration);

    if (parseResult.Errors.Count != 0)
    {
        return DisplayValidationErrors(configuration, parseResult);
    }
    else if (parseResult.FindResultFor("--help") is not null)
    {
        DisplayHelp(configuration, parseResult);
    }
    else if (parseResult.FindResultFor("--version") is not null)
    {
        DisplayVersion();
    }
    else if (parseResult.FindResultFor("[parse]") is not null)
    {
        DiagramInput(parseResult);
    }
    else if (parseResult.FindResultFor("[suggest]" is not null)
    {
        SuggestResult(parseResult); // this is for tab completion
    }
    else
    {
        InvokeCommand(configuration, parseResult, invocationResult),
    }
}

Personally I would prefer the try pattern:

public static int Run(Command root, string[] args, CommandLineConfiguration configuration = default)
{
    ParseResult parseResult = root.Parse(args, configuration);

    if (parseResult.Errors.Count != 0)
    {
        return DisplayValidationErrors(configuration, parseResult);
    }
    else if (parseResult.TryFindResultFor("--help", out SymbolResult? result))
    {
        DisplayHelp(configuration, parseResult);
    }
    else if (parseResult.TryFindResultFor("--version", out result))
    {
        DisplayVersion();
    }
    else if (parseResult.TryFindResultFor("[parse]", out result))
    {
        DiagramInput(parseResult);
    }
    else if (parseResult.TryFindResultFor("[suggest]", out result))
    {
        SuggestResult(parseResult); // this is for tab completion
    }
    else
    {
        InvokeCommand(configuration, parseResult, invocationResult),
    }
}

Giant if-else block does not look as pretty as switch, but I don't expect real world scenarios to be willing to customize every possible thing.
I've taken a look at the only middleware SDK is providing: https://github.com/dotnet/sdk/blob/b8f58bc235360597781de64f0c53190c80a68599/src/Cli/dotnet/Parser.cs#L163-L200

What it wants to achieve is custom error reporting for one command. This would be:

public static int Run(Command root, string[] args, CommandLineConfiguration configuration = default)
{
    ParseResult parseResult = root.Parse(args, configuration);

    if (parseResult.Errors.Count != 0 && parseResult.CommandResult.Command.Name == "specialName")
    {
        return DisplayCustomError(configuration, parseResult);
    }
    else
    {
        parseResult.Invoke();
    }
}

Chaining

Having multiple actions that need to be invoked requires implementing chaining mechanism.

In my opinion, the fewer public types we expose (CliAction, ICommandHandler), the better as we can change the implementation without affecting the public contract.

What I am trying to say is that in my opinion Command, Directive and maybe Option should be exposing only one way to set sychronous and asychronous delegates and not exposing any property that returns something else.

public class Command 
{
    public Func<ParseResult, int> SynchronousHandler { get; set; }
    public Func<ParseResult, CancellationToken, Task<int>> AsynchronousHandler { get; set; }
}

And the actual chaining should be done by Invoke and InvokeAsync and being an implementation detail.

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

Successfully merging this pull request may close these issues.

2 participants