serenity-rs / poise

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

Implement get_replied_msg_author #259

Closed 1Kill2Steal closed 3 months ago

1Kill2Steal commented 3 months ago

These are the following changes made

  1. Implemented in /src/structs/context.rs:
  1. Added an example usage (/examples/get_replied_user/main.rs)
GnomedDev commented 3 months ago

I don't see why this should be in poise, instead of in a utility module in your own bot? It isn't that complicated and doesn't rely on anything internal to poise.

kangalio commented 3 months ago

Hmm, is this functionality worth yet another function to keep around? If anything, it should be a method on Context instead of a free standing function.

Even then, the function essentially only does ctx.msg.referenced_message?.author plus some required boilerplate. How does this utility function help you in your bot codebase(s)?

1Kill2Steal commented 3 months ago

I agree with the function not being complicated to implement, however I think it'll help out to add it in a more concise way for the sake of simplicity. It may be just my code base but I happen to use this pattern a lot with the way I retrieve a user.

Hmm, is this functionality worth yet another function to keep around? If anything, it should be a method on Context instead of a free standing function.

The method idea does sound like a very way to work around it. The boilerplate can just be shortened to another method because the context needs to be matched to a message context first (because ? only works on Result types) so the ctx.msg.referenced_message?.author needs to be extended to something like:

match ctx {
    poise::Context::Prefix(msg_ctx) => msg_ctx
        .msg
        .referenced_message
        .as_ref()
        .map(|x| x.author.clone()),
    _ => None,
}

It'd be even better if the author doesn't need to be cloned but I don't know how to do it without the clone.

My main motivation is that I've also seen a lot of bots which also use the message replies as a way to select a user for an interaction so a native library method which can shorten the boiler plate would be a helpful feature.

The way I did the implementation doesn't seem to be that good so if there's nothing more to reply on feel free to close this PR. Perhaps a poise::Context method along the lines of ctx.get_replied_msg_author() (returning an Option<User>) would be a better implementation for the same feature.

Thank you both for the feedback and I wish you have a nice day/evening!

GnomedDev commented 3 months ago

Okay, I understand your point but I don't feel like this is a worthwhile addition, especially seeing that prefix commands are definitely in the minority of usages (I have statistics on this if you want to see) adding boilerplate for a prefix-only mechanism doesn't seem worthwhile to contain in the library.

This is not to say I would be against kanga merging this, but I'm not going to.

1Kill2Steal commented 3 months ago

Okay, I understand your point but I don't feel like this is a worthwhile addition, especially seeing that prefix commands are definitely in the minority of usages (I have statistics on this if you want to see) adding boilerplate for a prefix-only mechanism doesn't seem worthwhile to contain in the library.

This is not to say I would be against kanga merging this, but I'm not going to.

Sure, I want to check out those statistics out of pure curiosity

GnomedDev commented 3 months ago

Take a look in the serenity discord server, I'll send them there.

1Kill2Steal commented 3 months ago

Hmm, is this functionality worth yet another function to keep around? If anything, it should be a method on Context instead of a free standing function.

Even then, the function essentially only does ctx.msg.referenced_message?.author plus some required boilerplate. How does this utility function help you in your bot codebase(s)?

Alright, I changed the PR to match a method implementation for the context struct instead of a separate function (it's nice that it works without cloning the referenced message author as well), I'm leaving the merge up to you now.

1Kill2Steal commented 3 months ago

Alright, I'm dumdum and I found out that this can be done without cloning by just returning a &User with the following utility:

pub async fn get_replied_user(ctx: Context<'_>) -> &poise::serenity_prelude::User {
    let poise::Context::Prefix(msg_ctx) = ctx else {
        return ctx.author();
    };
    let ref_msg = msg_ctx.msg.referenced_message.as_deref();
    ref_msg.map_or(ctx.author(), |x| &x.author)
}

Yeah, this PR isn't necessary since it can be done like that so I'm going to close it.

I'm sorry for the unneeded inconvenience.