Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement get_replied_msg_author #259

Closed
wants to merge 12 commits into from

Conversation

1Kill2Steal
Copy link

@1Kill2Steal 1Kill2Steal commented Mar 30, 2024

These are the following changes made

  1. Implemented in /src/structs/context.rs:
  • Implemented the context method which checks if the user replied to a message (if yes it returns a reference to the replied message author) otherwise it returns a reference of the context author.
  1. Added an example usage (/examples/get_replied_user/main.rs)
  • It's quite similar to the /age example, It just prioritizes the selected user option and if there's none it tries to get the referenced message author and if there's none it just gets the context author.

It works by checking if there's a user (if there's none it returns None)
afterwards it checks if there's a replied message and if there is it
gives the replied message author, otherwise it gives the default message
author (configurable to be None if wanted).
@1Kill2Steal 1Kill2Steal changed the title Implement get_replied_user Implement get_replied_user Mar 30, 2024
@GnomedDev
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

Set it as a method for the context struct and removed the prior function
implementation.
@1Kill2Steal
Copy link
Author

1Kill2Steal commented Mar 31, 2024

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 1Kill2Steal changed the title Implement get_replied_user Implement get_replied_msg_author Apr 1, 2024
@1Kill2Steal
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants