-
Notifications
You must be signed in to change notification settings - Fork 121
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
Conversation
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).
The purpose is to save resources in the evaluation of the variable, if there's already a user there's no need for additional computation for the replied user retrieval.
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. |
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 |
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.
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 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 Thank you both for the feedback and I wish you have a nice day/evening! |
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 |
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.
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. |
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. |
These are the following changes made
/src/structs/context.rs
:/examples/get_replied_user/main.rs
)