Subcommands

[Tylertron1998]

How do we handle subcommands? Right now I'm leaning towards not handling them. One of the main goals of this library is simplicity - and it isn't hard to handle subcommands user-end either:

public class ModerationCommand : ICommand<ModerationType, User, string>
{
    public ValueTask RunAsync(ModerationType type, User user, string reason = "")
    {
        return type switch
        {
            ModerationType.Ban => BanAsync(user, reason),
            ModerationType.Kick => KickAsync(user, reason)
            // etc
        }
    }

    private async ValueTask BanAsync(User user, string reason) => user.BanAsync(reason);
    private async ValueTask KickAsync(User user, string reason) => user.KickAsync(reason);
}

Obviously this is a simple example - and it does pose one issue: all subcommands need to have the same usage. Another idea is "overloaded" commands, where the command is picked based on the name and the usage - so you can have several commands with the same name, but different usages'. This could be annoying for code duplication, however, and would likely be bad for performance.

[kyranet]

How do you propose handling subcommands with different arguments? Let's say you have a set <string> <string> and a reset <string>, even show [string].

[Tylertron1998]

It wouldn't be possible with the proposed method - at least not fully. That would be possible with optional types, i.e. the overall command type would be [string] [string] - which could then resolve into <string> <string>, <string> and [string] - but more complex types would not be possible.

This is where I'd lean towards the mentioned overloaded commands - which might be better long term anyway.

[Stitch07]

Since the library is generating a lot of if statements anyway, what's the issue with adding a subcommand API I suggested earlier?

public class ConfCommand: ICommand
{

    [SubCommand]
    public async ValueTask Set(Context ctx, string key, string value)
    {      
    }

    [SubCommand(Default=True)]
    public async ValueTask Show(Context ctx, string key = "")
    {      
    }

}

[Tylertron1998]

@soumil07 so, the reason why this wasn't specifically considered is because the current setup avoids "floating methods" - there is a specific interface you implement (ICommand<T...>) which then forces you to make a method of type ValueTask RunAsync(T...) - and that method is the method to be executed by the command handler. Using attributes means that we'd instead consider a method as a command method if they have that attribute - rather then the strict type/signature it represents. It also means less strong typing - currently, you implement an interface, and the generic type arguments for that interface are the commands arguments - and that's also reflected in the method.

However, I've come to realize, we can have the best of both worlds. Consider:

public class ConfCommand : ICommand<string, string>, ICommand<string>
{
    [SubCommand("set")]
    public async ValueTask RunAsync(string key, string value) {} // this is required to exist because of the implementation of ICommand<string, string>

    [SubCommand("show")]  // and this, because of the implementation of ICommand<string>
    public async ValueTask RunAsync(string key) {}
}

Now, this leaves only one "problem" which is you cannot have two (or more) subcommands with the exact same usage. Though, honestly, if you have two subcommands with the exact same usage, IMO they aren't subcommands, that's just a command with two options.

CC @kyranet

[kyranet]

Hint, two subcommands with the same args but entirely different behaviour:

set <string key> <string value>
remove <string key> <string value>

[Tylertron1998]

And this is where I'd argue really, that isn't two "different" commands, that's just a command with two different options

public class ConfCommand : ICommand<ConfType, string, string>
{
    public async ValueTask RunAsync(ConfType type, string key, string value)
    {
        return type switch
        {
            ConfType.Set => SetAsync(key, value),
            ConfType.Remove => RemoveAsync(key, value)
        }
    }
}

this also encourages users to move more logic outside of commands, and abstract them into contained classes/methods, for better testbility.

[Tylertron1998]

Realistically, the only "use" for subcommands I can see is a command under the same name, that takes two different usages' - if it has the same name, and has the same usage, why not just handle that yourself with a simple enum/switch case? It also means you don't have two different sets of logic pushed into a single method - which again, is great for code readability and testing.

[Tylertron1998]

After some internal discussion in discord with @kyranet - more complex types (i.e. what if there are several subcommands, some share a common type, and some don't) can't really be handled by this. Some more thought is definitely needed.

[Tylertron1998]

After some more thoughts and discussion, I believe the best way would be to mix this with this - they're obviously not going to be compatible, but that's not an issue. Having extremely complex subcommand types where there are both subcommands with and without shared usage is overly complex.