Concrete Command Definitions

CanIGetaPR commented 3 years ago

[x] I have checked for similar issues.
[x] I have updated to the [latest JDA version][download].
[x] I have checked the branches or the maintainers' PRs for upcoming features.

Feature Request

Define commands in a descriptive way that supports type safety when retrieving argument values. This would potentially create a high level abstraction over top of what JDA is forwarding from the Discord API, so I understand if this is rejected.

Reasoning

I've always been excited when I heard slash commands are coming with pre-defined parameters.

Now it's awesome that JDA has them implemented but it's not hitting that SPOT for me. When it comes to Java, I love the type safety. Code in a way which the compiler can verify that you haven't made any typos, and have your code structured, easy to re-use, easy to profile and verify. More towards concrete typing, object oriented.

problem image

In the image above, two things there make me feel that the code isn't really embracing what Java is about. And it's more something you'd see in a dynamic language. I get it, it's a quick example to show how slash commands can be implemented, but I don't want it to be the only way because:

Re-typing command names by hand can lead to typos that can't be found until runtime,
Forgetting what type your option is and choosing the wrong accessor is another bug you won't find until runtime.

...And these two don't make sense to have, especially in Java; because we've already spent time and effort defining our commands in the start of the example.

Possible To-Do:

[ ] Add command skeleton interface
[ ] Add functionality to register an implemented command skeleton
[ ] Add type safety (type parameter implementation) to OptionData / OptionMapper
[ ] Add the ability to deal with an incoming slash command usage by referencing the command skeleton

Possible Example of Type Safe Option

This original example code...

OptionMapping option = event.getOption("del_days");
if (option != null) // null = not provided
      delDays = (int) Math.max(0, Math.min(7, option.getAsLong()));

... Could end up looking similar to ...

process() {
    int delDays = (int) Math.max(0, Math.min(7, this.getDeleteOption());
}

Original:

new OptionData(OptionType.USER, "user", "The user to ban")

After:

new UserOption("user", "The user to ban")

Where, UserOption extends OptionData<User>

CommandData would be a concrete class that defines the command and also is used as a command instance for processing.

freya022 commented 3 years ago

This code does not belong to JDA, JDA is a low-level enough library (unlike some others such as discord.py which provides a command framework out of the box) and is intended to be kept low level, providing everything a user could need to build higher level APIs

You can however do your own solution as a 3rd party library, or use an existing one

Also having errors in such thing as a compile time error seems difficult, what should be more doable is a runtime error, but at startup, not when a command is used, this way errors are "catched" fast

Andre601 commented 3 years ago

I really doubt anyone forgets the arguments they themself typed. And if you're that forgetful should you perhaps take a break.

I can't even see any real way here how commands and especially arguments, which allow pretty much any names possible, more type-safe.

What you ask for is that JDA remembers every single argument of every single command you created, which can be up to 100 commands in total. JDA does nothing more than sending the generated command info to Discord and patiently waits for Discord to send slash command events back, nothing more. Storing each command and their optionmappings as values in cache seems really pointless to have.

And you could for your own convenience just make a class with some static methods to retrieve data. Here is an example I use:

public class CommandUtil {

    public static String getString(SlashCommandEvent event, String key, String def) {
        OptionMapping option = event.getOption(key);
        if(option == null)
            return def;

        return option.getAsString();
    }

    public static long getLong(SlashCommandEvent event, String key, long def) {
        OptionMapping option = event.getOption(key);
        if(option == null)
            return def;

        return option.getAsLong();
    }
}

If you really fear to misstype a option name or similar, then how about making a final String in the class to use? Like here is an example with JDA-Chewtils' SlashCommand setup:

public class BanCommand extends SlashCommand {

    private final String USER = "user"; // Used for the user options
    private final String DEL_DAYS = "del_days"; // Used for the del_days options

    public BanCommand() {
        this.name = "ban";
        this.help = "Bans the user";

        this.options = Arrays.asList(
            new OptionData(OptionType.STRING, USER, "The user to ban. Either name, or ID", true),
            new OptionData(OptionType.INTEGER, DEL_DAYS, "How many days worth of messages should be removed? Up to 7 days possible")
        );
    }

    @Override
    protected void execute(SlashCommandEvent event) {
        OptionMapping userOption = event.getOption(USER);
        OptionMapping daysOption = event.getOption(DEL_DAYS);

        // Handle the stuff
    }
}

If this is not what you were explaining would I appreciate some more info about this... I had a hard time really understanding your idea.

CanIGetaPR commented 3 years ago

The example code I posted barely reflects my suggestion so I understand the confusion. Hopefully I can get a PoC in a PR at some point.

MinnDevelopment commented 3 years ago

It is very unlikely that such a databind system will ever be added to this library, since it is a very opinionated design. Feel free to make an extension library that adds this functionality, but the base library is supposed to be flexible and low level. You also shouldn't be registering your commands via updateCommands on every startup. Commands persist between sessions and should only be updated once they change.

CanIGetaPR commented 3 years ago

It is very unlikely that such a databind system will ever be added to this library, since it is a very opinionated design. Feel free to make an extension library that adds this functionality, but the base library is supposed to be flexible and low level. You also shouldn't be registering your commands via updateCommands on every startup. Commands persist between sessions and should only be updated once they change.

You are right. Most of the suggestions are out of scope for the API. I got mixed up when I read the Slashbot Example because I thought JDA was already trying to implement a command system. But after deeper analysis, JDA just wants to get the data from the programmer to Discord as efficiently as possible.

CanIGetaPR commented 2 years ago

It is very unlikely that such a databind system will ever be added to this library, since it is a very opinionated design. Feel free to make an extension library that adds this functionality

This is what I ended up making if anyone's interested. I'll be updating it again sometime soon when I get back into developing my bot.

https://gitlab.com/pawnstudios/libraries/discord-commands

discord-jda / JDA