yagpdb
yagpdb copied to clipboard
templates: add getAllMessageReactions
Expose the MessageReactions
endpoint, which yields the users that reacted with an emoji on a message.
The MessageReactions
endpoint is paginated (with before
and after
) parameters. This PR opts to abstract that out for the user by looping internally until all users are fetched.
To prevent abuse getAllMessageReactions
is limited to one use per command, and is further limited to fetching 5000 users at maximum (which corresponds to 50 API calls.)
Tested to the best of my ability on a self-host, but I sadly don't have > 100 people to test with so it's possible the inner loop has bugs.
Some things to consider:
- Should this function be named
getAllMessageReactions
? It aligns with the API endpoint but I'm afraid the name may suggest that it fetches all message reactions (i.e. emojis.) Should we be more clear and name it something likegetAllReactedUsers
, or is this not a concern? - Do the limits here make sense? Presently,
getAllMessageReactions
is limited to one use per command and counts as one API call. The problem is that -- being a paginated endpoint -- an unknown number of API requests is needed to fetch all reactions. The implementation here makes 50 API calls at max which are not counted toward the total API call counter, to keep the maximum number of reactions returned constant thereby allowing for more reliable error handling within custom commands. Is this behaviour acceptable? (Read the comments below for further context & discussion.)
Shouldn't the function IncreaseCheckGenericAPICall()
be called everytime an API call is made? Instead of only once at the beginning.
Perhaps check that inside the loop and if it hits the limit return what has been fetched thus far and stop the calls?
Re: calling IncreaseCheckGenericAPICall
per-iteration, I believe the current approach is preferable as it provides a fixed limit on how many reactions can be fetched, whereas your suggestion would make it dynamic. Having the limit be fixed and not dependent on how many API calls have been made previously allows users to check whether calling getAllMessageReactions
would hit the limit by simply checking the reaction count and output a user-friendly error in such a case, whereas a dynamic limit would make robust error detection much more difficult.
I'm willing to be convinced otherwise, though. What do you think?
I see your point and I agree with it. But I think it is fair to bring this to the dev's attention, reason being API Calls on CC's are hard capped to 100. With this function users could potentially be doing 150 API Calls per CC, a 50% increase from the previous limit.
If this is ok for YAG or no, I'm not the one to tell. But, again, I agree with you here, and I think the way you handled IncreaseCheckGenericAPICall
is the best for the function per se, but I don't know if it is the best for "CCs context" as a whole or not.
I think your points make sense (but I'm still not convinced either way, unfortunately :P)
Gonna mark this as draft to make sure it isn't merged for now & wait for some other opinions.