serenity-rs / poise

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

The Default impl for Command has unnecessary bounds #212

Closed Makefile-dot-in closed 9 months ago

Makefile-dot-in commented 10 months ago

The Default impl on the Command struct has the following bounds, autogenerated by derivative:

impl<U, E> Default for Command<U, E>
where U: Default,
      E: Default

However, U and E don't actually need to have a Default implementation, since none of the fields in Command require construction of either to have a Default impl, and there isn't any reason why they ever would in the future - user data isn't stored in Command and E is only returned as the Err variant of Result, where constructing the Ok variant is always possible to get a default value. It would also be silly in most cases for the error to have a Default impl, and most error types don't, making this impl useless for nearly all cases.

My usecase for this is to have a command for which the subcommands are determined from a const variable in a different module - something like this:

// lib.rs
pub const ADMIN_COMMANDS: &[fn() -> poise::Command<Data, Error>] = &[ 
    log::log,
    remind::reminders_adm,
];

// admin.rs
use poise::Command;

pub fn admin() -> Command<Data, Error> {
    Command {
       name: "admin",
       subcommands: ADMIN_COMMANDS
           .map(|f| f())
           .collect(),
       slash_action: Some(|_| Box::pin(async move { Ok(()) })),
       subcommand_required: true,
       ..Default::default()
    }
}

whereas now I have to do something like this:

use poise::{Command, Context};

pub fn admin() -> Command<Data, Error> {
   #[poise::command(slash_command, rename = "admin")
   fn inner(_: Context<'_, Data, Error>) -> Result<(), Error> { Ok(()) }
   Command {
     subcommands: ADMIN_COMMANDS
        .map(|f| f())
        .collect(),
       subcommand_required: true,
      ..inner()
   }
}

I suppose in this case this isn't that big of a deal though (though it might matter more in other cases).

kangalio commented 10 months ago

Good point. I'm open for PRs