-
-
Notifications
You must be signed in to change notification settings - Fork 465
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
feat: Add cooldown
and max_concurrency
to SlashCommandGroup
#2091
feat: Add cooldown
and max_concurrency
to SlashCommandGroup
#2091
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2091 +/- ##
==========================================
- Coverage 33.13% 33.11% -0.02%
==========================================
Files 97 97
Lines 19141 19151 +10
==========================================
Hits 6342 6342
- Misses 12799 12809 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Signed-off-by: Om <92863779+Om1609@users.noreply.github.com>
Signed-off-by: Om <92863779+Om1609@users.noreply.github.com>
On third thought, since cooldowns and max concurrency can't be used by groups or affect their subcommands, you don't need to implement them. For the confusing formatting, it accounts for the fact that the og pr (#691) removed inheritance of these features, so it just checks one to filter out group objs. Instead of reimplementing cooldowns and max concurrency, you could just reformat the code to something like this: if not getattr(self, "_max_concurrency", None):
# For this application, context can be duck-typed as a Message
await self._max_concurrency.acquire(ctx) # type: ignore # ctx instead of non-existent message
if hasattr(self, "_buckets"):
try:
self._prepare_cooldowns(ctx)
await self.call_before_hooks(ctx)
except:
if not getattr(self, "_max_concurrency", None):
await self._max_concurrency.release(ctx) # type: ignore # ctx instead of non-existent message
raise |
They actually do control the subcommands. By setting a cooldown/max_con on the group, each individual subcommand gets a shared cooldown/max_con. So if you had set 1 token per minute as the cooldown on the group, after first cmd invoke, you can't invoke any other command from the parent group for a minute. This also works with subgroups. So this is actually a valid working feature. |
Co-authored-by: Middledot <78228142+Middledot@users.noreply.github.com> Signed-off-by: Om <92863779+Om1609@users.noreply.github.com>
I actually confused this and the Invokable pr, because I addressed this there. Prefixed group commands don't have the same behaviour. Their group checks and subcommand checks are independent from each other, so I say the same should be for app commands |
Oh how?
The individual cmd's cooldown and max concurrency still apply. You could have 1 per 1 min cooldown on group, while also having 1 per 5 mins on a certain subcommand. (placeholder values for example purposes) Eg- So now you can invoke only 1 of any of the subcommands in a minute. If the command invoked was I hope this makes sense. I should also re-test if the logic I mentioned above actually holds true. |
Oh my bad, I confused it again. What I was talking about were checks of parents being run by subcommands. Cooldowns etc are managed differently. What you're talking about is an interesting use case that I never realized, plus it's consistent with prefixed commands, so yeah, nevermind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
Adds
cooldown
andmax_concurrency
toSlashCommandGroup
. SinceSlashCommandGroup
cannot be created using a decorator, these are created manually and passed directly to the init.Also fixes #1935 by cleaning up the
ApplicationCommand.prepare
logic. (PR based on discussion in #2057)Information
examples, ...).
Checklist
type: ignore
comments were used, a comment is also left explaining why.