serenity-rs / poise

Discord bot command framework for serenity, with advanced features like edit tracking and flexible argument parsing
MIT License
630 stars 112 forks source link

feature: default subcommand when no arguments provided #169

Closed sadorowo closed 1 year ago

sadorowo commented 1 year ago

Hello, I have enhanced the library with an interesting function. Let's create an example of a command with subcommands.

#[poise::command(prefix_command, slash_command, subcommands("show", "edit"))]
pub async fn configuration(ctx: Context<'_>) -> Result<(), Error> {
   // Before this change, when no arguments were given, this code was executed.
}

#[poise::command(prefix_command, slash_command)]
pub async fn show(ctx: Context<'_>) -> Result<(), Error> {
   // ...
}

#[poise::command(prefix_command, slash_command)]
pub async fn edit(ctx: Context<'_>) -> Result<(), Error> {
   // ...
}

But I created a simple parameter in poise::command proc macro. From now, you can use default_subcommand parameter. No more repeating code! default_subcommand sets the default subcommand; when no subcommand is given, the default subcommand will be run

Example:

#[poise::command(prefix_command, slash_command, subcommands("show", "edit"), default_subcommand = "show")]
pub async fn configuration(ctx: Context<'_>) -> Result<(), Error> {
   // After this change, the code will NEVER be executed.
   // You only need to return Ok because of type checking.
   Ok(())
}

#[poise::command(prefix_command, slash_command)]
pub async fn show(ctx: Context<'_>) -> Result<(), Error> {
   // From now it it the default subcommand.
   // It will be fired in these e.g. cases:
   // .configuration
   // .configuration show
}

#[poise::command(prefix_command, slash_command)]
pub async fn edit(ctx: Context<'_>) -> Result<(), Error> {
   // ...
}

I think this would be a great feature, you won't have to repeat your code. I've tested a few cases and everything works fine, although I would recommend checking my code more thoroughly for possible errors/shortcomings before accepting the PR.

Errors that I have implemented:

- #[poise::command(slash_command, subcommands("a", "b"), default_subcommand = "a")]
-                 ^^ WRONG! `prefix_command` is NOT added here; default_subcommand ISN'T SUPPORTED
-                 "`default_subcommand` is supported only in `prefix_command`, however you didn't enabled prefix_command"

- #[poise::command(prefix_command, default_subcommand = "a")]
-                                  ^^ WRONG! You specified `default_subcommand` without specifying `subcommands`.

+ Also, type check for `default_subcommand` is implemented, so you can't simply pass anything like `default_subcommand = someinvalidliteral`. This will return an proc macro error.

Feel free to edit my code, it may be not perfect.

sadorowo commented 1 year ago

bug discovered; you can pass some subcommand to default_subcommand even if subcommands does not contain this value

EDIT: added a review

kangalio commented 1 year ago

Thank you for taking the time to contribute a feature!

I wonder if the additional API surface and maintenance is worth it... Especially because text commands are used less and less

You can already do this:

#[poise::command(prefix_command, slash_command, subcommands("show", "edit"))]
pub async fn configuration(ctx: Context<'_>) -> Result<(), Error> {
   show_impl().await;
}

async fn show_impl(ctx: Context<'_>) -> Result<(), Error> {
   // ...
}

#[poise::command(prefix_command, slash_command)]
pub async fn show(ctx: Context<'_>) -> Result<(), Error> {
   show_impl().await
}

#[poise::command(prefix_command, slash_command)]
pub async fn edit(ctx: Context<'_>) -> Result<(), Error> {
   // ...
}
sadorowo commented 1 year ago

Thank you for taking the time to contribute a feature!

I wonder if the additional API surface and maintenance is worth it... Especially because text commands are used less and less

You can already do this:

#[poise::command(prefix_command, slash_command, subcommands("show", "edit"))]
pub async fn configuration(ctx: Context<'_>) -> Result<(), Error> {
   show_impl().await;
}

async fn show_impl(ctx: Context<'_>) -> Result<(), Error> {
   // ...
}

#[poise::command(prefix_command, slash_command)]
pub async fn show(ctx: Context<'_>) -> Result<(), Error> {
   show_impl().await
}

#[poise::command(prefix_command, slash_command)]
pub async fn edit(ctx: Context<'_>) -> Result<(), Error> {
   // ...
}

that's true, prefix commands are going to be deprecated in favor of slash commands, I will leave it up to you to decide whether to accept/reject this PR

sadorowo commented 1 year ago

I created sth better I think.