Adding TriggerCooldown #12

TeRacksito · 2023-04-25T18:58:55Z

Adding Trigger Cooldowns as mentioned in the discord.

A useful feature to be able to trigger specific cooldown regardless of any other conditions. If you need to trigger a cooldown instantaneously to deny anyone affected by the bucket of the cooldown to actually use the command, you just need one line of code now!

First you need to define the cooldown, almost the same as shared cooldowns (But is not a shared cooldown!)

my_trigger_cooldown = TriggerCooldown(1, 7, SlashBucket.author)

Then you need to decorate the function you want to apply the cooldown.

@my_trigger_cooldown
async def test_command(self, interaction: Interaction):

my_trigger_cooldown has to be on the same scope!

Trigger the cooldown on any line inside your command at any time you want!

await my_trigger_cooldown.trigger(40)

If you want to use this inside cogs, its the same, but using self to trigger the cooldown because you need to define the cooldown instance in the same scope as the command, that means inside the cogs class!

class TestCommand(commands.Cog):
  my_trigger_cooldown = TriggerCooldown(1, 7, SlashBucket.author)

  @nextcord.slash_command()
  @my_trigger_cooldown
  async def test_command(self, interaction: Interaction):
      await self.my_trigger_cooldown.trigger(40)

Skelmis

Few overarching comments not specific to any points in the code:

Please move the trigger cooldown to it's own file
Please add test cases for your new code
Please add these all new items to the documentation
Please add the ability to use these cooldowns directly via a decorator such as @trigger_cooldown(...) as well as via shared cooldowns

Has black been run over your code changes with the default styling? If not, can you do this please

Skelmis · 2023-04-25T20:59:13Z

cooldowns/cooldown.py

+    - Normal cooldown. The same cooldown as @cooldowns.cooldown()
+    - Trigger cooldown. A secondary cooldown that can only be activate
+    with `.trigger()`


Suggested change

- Normal cooldown. The same cooldown as @cooldowns.cooldown()

- Trigger cooldown. A secondary cooldown that can only be activate

with `.trigger()`

- Normal cooldown. The same cooldown as :py:func:`cooldowns.cooldown`

- Trigger cooldown. A secondary cooldown that can only be activated with `.trigger()`

Skelmis · 2023-04-25T21:00:48Z

cooldowns/cooldown.py

+    Usage
+    -----
+        - First create an instance of TriggerCooldown() with
+        the desired parameters.
+
+        ```
+        trigger_cooldown = cooldowns.TriggerCooldown(1, 5, cooldowns.SlashBucket.author)
+        ```
+
+        - Then add the instance as a decorator to your command!
+
+        ```
+        @nextcord.slash_command()
+        @trigger_cooldown
+        async def command():
+        ```
+
+            The instance has to be defined in the same scope as the decorator!
+            Now, `command()` has applied a normal cooldown of `1 limit` and
+            `5 time_period`, as we defined it.
+
+        - Finally, inside your command, you can `trigger` the trigger cooldown:
+
+        ```
+        async def command():
+            # Do things
+            trigger_cooldown.trigger(30)
+            # You can still do things after this.
+            # Even you can `interaction.send()`.
+        ```
+
+            From the moment when the cooldown was triggered by `.trigger(30)`, every
+            single call to this command within 30 seconds will raise CallableOnCooldown!


This is better suited to an examples entry in the documentation or in the decorators docstring

I totally understand the reasons you may want the "ability to use these cooldowns directly via a decorator such as @trigger_cooldown(...) as well as via shared cooldowns" But here is a problem.

To trigger a cooldown, you need an instance of it. The only way I can figure out doing this is by creating an instance of the trigger cooldown and then using it as a decorator.

As far as I know, it is not possible to create a direct decorator because there is no instance to "trigger". When you decorate a function with, for example, @trigger_cooldown(...) you are just wrapping that function with the cooldown. The same happens with shared_cooldowns. Trigger cooldowns have their own behavior, they have to be an object, an instance, to be callable and modifiable. A trigger cooldown does not just "wrap" a function, but also activates when the function dictates to activate.

The same happens with shared cooldowns, if we want to make trigger cooldowns shareable, then a new "sharing" method has to be created so that triggering is matched with the function that triggers.

My opinion is that one thing is just a trigger cooldown, and another thing is implementing methods like sharing and others.

So, the conclusion is that, as no direct cooldown decorator can be made, no docstring directly to the decorator can be created, because the decorator is an instance, a variable.

True, however, when storing against an id someone could use the utility objects to fetch the cooldown instance via id as and where required for this functionality.

I would still like to see decorators, but with those comments in mind you may just need to make the cooldown id enforced so they can be fetched from the global state everytime when using methods such as this

I will keep that in mind to search for a way to do it with get_cooldown().

I meant to have get_cooldown get the TriggerCooldown instance, not the underlying cooldowns which is what appears to occur now

cooldowns/cooldown.py

Skelmis · 2023-04-25T21:02:35Z

cooldowns/cooldown.py

+        if cooldown_id:
+            utils.shared_cooldown_refs[cooldown_id] = self.cooldown


You'll need to figure out a way to nicely make this work for both. You define two cooldowns but only store 1 in the global store which may lead to issues

The workaround that I have done is to store both cooldowns with the same ID only if the user defines an ID.

Remember that trigger cooldowns can not be shared. This could be added in the future because it will require an entirely new implementation of shared cooldowns only for trigger cooldowns.

Talking again about IDs:

# Normal Cooldown self.cooldown = Cooldown( limit= self.limit, time_period= self.time_period, bucket= self.bucket, cooldown_id= self.cooldown_id, # <-- Here... check= self.check ) # Trigger Cooldown self.trigger_cooldown = Cooldown( limit= 1, time_period= self.time_period, bucket= self.bucket, cooldown_id= self.cooldown_id, # <-- Here... check= self.check

This means that, if the user wants to clean up the cooldown, should be able to do it with the ID. It will clean both cooldowns, but for me, that makes sense because they are together. These two cooldowns will always work in the same function, if you want to clean up one cooldown is probably because you want to remove the cooldown from your function, regardless of if it is the normal cooldown or the trigger cooldown.

In my opinion, it is not necessary to have separate ID for both cooldowns, as they work like twins. But, if separated IDs are wanted, then just add a second argument to the constructor:

class TriggerCooldown: def __init__( self, limit: int, time_period: Union[float, datetime.timedelta], bucket: Optional[CooldownBucketProtocol] = None, *, normal_cooldown_id: Optional[Union[int, str]] = None, # <-- Here... trigger_cooldown_id: Optional[Union[int, str]] = None, # <-- Here... check: Optional[MaybeCoro] = default_check, ):

And add them to their respective cooldowns, but again, I do not see this as useful and it could lead to other unknown errors.

So just throw self into the global cooldown state..? It should work from what I can tell and provides access to both objects via the same ID.

Side note, I don't see a reason they can't be shared. Current implementation sure, but at a higher level I don't see it.

A goal I'd like to see is the ability to decorate multiple functions and share them. It'd be nice for applications such as grouped commands, etc

I'll keep in mind it to make them shareable.

Skelmis · 2023-04-25T21:04:42Z

cooldowns/cooldown.py

+            )
+        # Links the cooldowns to the given function.


Suggested change

)

# Links the cooldowns to the given function.

)

# Links the cooldowns to the given function.

Skelmis · 2023-04-25T21:05:15Z

cooldowns/cooldown.py

+                    return result
+                # If not, untrigger the cooldown.


Suggested change

return result

# If not, untrigger the cooldown.

return result

# If not, untrigger the cooldown.

Skelmis · 2023-04-25T21:05:23Z

cooldowns/cooldown.py

+                    self.triggered = False
+            # If the cooldown is not triggered.


Suggested change

self.triggered = False

# If the cooldown is not triggered.

self.triggered = False

# If the cooldown is not triggered.

Skelmis · 2023-04-25T21:06:19Z

cooldowns/cooldown.py

+                else:
+                    result = await func(*args, **kwargs)
+                return result
+        # Return the decorator.


Suggested change

# Return the decorator.

cooldowns/cooldown.py

Skelmis · 2023-04-29T23:58:54Z

tests/test_trigger_cooldown.py

+    assert await test_1() == 1
+    assert await test_2() == 2


Would this not trigger the cooldown? It's two calls to a cooldown which is 1/0.3 and I doubt it takes longer then 0.3 to execute both

Oh, you are totally right. My apologies for the inconvenience. I had to go on a trip and I did it too fast. Second assert has to be removed.

Skelmis · 2023-07-20T11:41:07Z

cooldowns/trigger_cooldown.py

+        # Triggers the Cooldown leaving bucket.current = 0
+        frame = inspect.currentframe().f_back
+        _, _, _, values = inspect.getargvalues(frame)
+        args = tuple(values.values())


I disagree with implicit argument fetching here, I'd prefer if an end user provided the arguments to trigger for a few reasons:

This doesn't handle nested situations or situations where trigger is called outside the context of the command it decorates

This doesn't ideally handle buckets, I.e. if you want to trigger for a specific bucket

If this trigger is meant to affect every call regardless, why does it pass args?

Skelmis · 2023-07-20T11:42:58Z

cooldowns/cooldown.py

+    Usage
+    -----
+        - First create an instance of TriggerCooldown() with
+        the desired parameters.
+
+        ```
+        trigger_cooldown = cooldowns.TriggerCooldown(1, 5, cooldowns.SlashBucket.author)
+        ```
+
+        - Then add the instance as a decorator to your command!
+
+        ```
+        @nextcord.slash_command()
+        @trigger_cooldown
+        async def command():
+        ```
+
+            The instance has to be defined in the same scope as the decorator!
+            Now, `command()` has applied a normal cooldown of `1 limit` and
+            `5 time_period`, as we defined it.
+
+        - Finally, inside your command, you can `trigger` the trigger cooldown:
+
+        ```
+        async def command():
+            # Do things
+            trigger_cooldown.trigger(30)
+            # You can still do things after this.
+            # Even you can `interaction.send()`.
+        ```
+
+            From the moment when the cooldown was triggered by `.trigger(30)`, every
+            single call to this command within 30 seconds will raise CallableOnCooldown!


I meant to have get_cooldown get the TriggerCooldown instance, not the underlying cooldowns which is what appears to occur now

Skelmis · 2023-07-20T11:51:22Z

cooldowns/trigger_cooldown.py

+    async def trigger(self, time_period: Union[float, datetime.timedelta]) -> None:
+        """|coro|
+
+        Trigger the Trigger Cooldown instantly. Has to be awaited.


Suggested change

Trigger the Trigger Cooldown instantly. Has to be awaited.

Trigger the Trigger Cooldown instantly.

Skelmis · 2023-07-20T11:52:53Z

tests/test_trigger_cooldown.py

+@pytest.mark.asyncio
+async def test_trigger_cooldown_triggering():
+    my_trigger_cooldown = TriggerCooldown(1, 0.3, CooldownBucket.all)
+
+    @my_trigger_cooldown
+    async def test_1():
+        await my_trigger_cooldown.trigger(20)
+        return 1
+
+    assert await test_1() == 1
+
+    await asyncio.sleep(0.4)
+    with pytest.raises(CallableOnCooldown):
+        await test_1()


Ideally also a test to ensure it does allow for subsequent calls after the timer expires

Adding TriggerCooldown

f2ddb0e

Skelmis requested changes Apr 25, 2023

View reviewed changes

Allowing shared trigger cooldowns

5f43942

Skelmis reviewed Apr 29, 2023

View reviewed changes

TeRacksito added 2 commits May 4, 2023 14:47

Solve minor misprints.

db1c551

Trigger cooldown fixed test

49f0b85

Skelmis requested changes Jul 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding TriggerCooldown #12

Adding TriggerCooldown #12

TeRacksito commented Apr 25, 2023

Skelmis left a comment

Skelmis Apr 25, 2023

Skelmis Apr 25, 2023

TeRacksito Apr 25, 2023

Skelmis Apr 25, 2023

TeRacksito Apr 25, 2023

Skelmis Jul 20, 2023

Skelmis Apr 25, 2023

TeRacksito Apr 25, 2023

Skelmis Apr 25, 2023 •

edited

Loading

TeRacksito Apr 25, 2023

Skelmis Apr 25, 2023

Skelmis Apr 25, 2023

Skelmis Apr 25, 2023

Skelmis Apr 25, 2023

Skelmis Apr 29, 2023

TeRacksito May 4, 2023

Skelmis Jul 20, 2023

Skelmis Jul 20, 2023

Skelmis Jul 20, 2023

Skelmis Jul 20, 2023

		if cooldown_id:
		utils.shared_cooldown_refs[cooldown_id] = self.cooldown

	Trigger the Trigger Cooldown instantly. Has to be awaited.
	Trigger the Trigger Cooldown instantly.

Adding TriggerCooldown #12

Are you sure you want to change the base?

Adding TriggerCooldown #12

Conversation

TeRacksito commented Apr 25, 2023

Adding Trigger Cooldowns as mentioned in the discord.

Skelmis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skelmis Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skelmis Apr 25, 2023 •

edited

Loading