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

Add command cooldowns #346

Merged
merged 3 commits into from
Apr 30, 2023
Merged

Add command cooldowns #346

merged 3 commits into from
Apr 30, 2023

Conversation

Blocksnmore
Copy link
Contributor

About

This command adds the commandOnCooldown event, along with a cooldown property on both CommandClient and Command

Status

  • These changes have been tested against Discord API or do not contain API change.
  • This PR includes only documentation changes, no code change.
  • This PR introduces some Breaking changes.

const commandCanBeUsedAt =
(userCooldowns.commands[command.name] ?? 0) +
((command.cooldown ?? 0) as number)
const globalCooldown = userCooldowns.global + this.cooldown
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't one of them override the cooldown or am I reading the code wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally felt that it makes more sense to have the cooldowns separate so that you don't have a minute global cooldown from running one command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it adds the command cooldown with the global cooldown...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.cooldown is the global cooldown being set in the client, command.cooldown is set in each individual command.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my 2 cents:
global cooldown should prevent any user from running command x before the cooldown expires.

For example: resource heavy commands and/or for minigames like having a command that can only be redeemed by one person every y hours.

/**
* Defines a cooldown in miliseconds, preventing the same user from running the command within the cooldown period
**/
command.cooldown?: number
/**
* Defines a global cooldown in miliseconds, preventing any user from running the command within the cooldown period
**/
command.globalCooldown?:number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my 2 cents: global cooldown should prevent any user from running command x before the cooldown expires.

For example: resource heavy commands and/or for minigames like having a command that can only be redeemed by one person every y hours.

/**
* Defines a cooldown in miliseconds, preventing the same user from running the command within the cooldown period
**/
command.cooldown?: number
/**
* Defines a global cooldown in miliseconds, preventing any user from running the command within the cooldown period
**/
command.globalCooldown?:number

I've added the global cooldowns and also renamed the previous cooldown to hopefully reduce confusion in regards to what it applies to.

Added the globalCooldown property for a bot wide cooldown and renamed the previous cooldown variable to hopefully reduce confusion
Copy link
Member

@Helloyunho Helloyunho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, finally got what you were trying to do

@Helloyunho Helloyunho merged commit b2f8089 into harmonyland:main Apr 30, 2023
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.

3 participants