Add CooldownContext struct to make Cooldowns usable in more situations #175

scottbot95 · 2023-07-09T06:31:56Z

Problem

I was wanting to implement cooldowns in some custom event handler, but unfortunately it is not practical to construct a poise::Context manually which is required to use the Cooldowns struct.

Proposal

This PR refactors Cooldowns slightly to add an extra layer of abstraction around the poise::Context to make Cooldowns easier to use outside of a prefix/application command.

kangalio

Thank you for this PR!

A few requests: Could you

add a comment above CooldownContext explaining that it was factored out of Context to make cooldowns usable outside a poise::Context context
target this PR to the next branch instead, since this is a breaking change
remove the From impls to convert (Prefix, Application)Context to CooldownContext in favor of a struct method called .cooldown_context() (see

poise/src/structs/context.rs

Line 74 in 5d02b87

context_methods! {

)

scottbot95 · 2023-07-09T16:56:24Z

Thanks for the quick review! I can make those suggested changes. I was kinda skeptical if the From implementations were really the best way to make it easy to create.

Do you have opinions on implementing From<serenity::Message>? I added that since that is the use-case I was planning to use this for but I could see the argument that such a helper is out of scope of the library and should just be done by the user.

scottbot95 · 2023-07-09T18:33:56Z

Updates made. The current branch on my fork is now really just the next branch cuz I didn't want to re-create the PR and lose some context. But who cares; I'll just nuke my current branch after this PR if I decide I care enough lol.

kangalio · 2023-07-09T21:01:33Z

Thank you for implementing my requests!

Do you have opinions on implementing Fromserenity::Message? I added that since that is the use-case I was planning to use this for but I could see the argument that such a helper is out of scope of the library and should just be done by the user.

I agree with the thought process. I think it's ok to keep the impl here, because it doesn't bother in the docs and you can trivially reimplement it if you don't know it exists

scottbot95 · 2023-07-09T23:09:18Z

Hmm of course there were several linter errors. Linters seem to be the bane of my existence ;) Some of the errors don't seem related to my change but I went ahead and made cargofmt/clippy happy anyway.

scottbot95 · 2023-07-09T23:41:24Z

Hmmm all those failures passed when I tried to replicate the commands locally on my Windows machine or in the WSL2. And the errors don't seem related to my code at all. Not sure how best to proceed from here.

Tangential question if you know: What was the reasoning behind using an OrderedMap for storing the cooldowns instead of just a std::HashMap? It doesn't seem like we ever actually care about that order is preserved. The only other thing I could think of is that maybe it's a performance/memory thing under the assumption that these maps will normally be small so it's better to search a list than to use HashMap? If so is that really an assumption we want to make? Seems like that would just be deciding ahead of time that this library will never be used at scale.

kangalio · 2023-07-10T11:57:42Z

You're right those failures don't seem related to this PR. Just plain code rot >:(

I checked and I actually can't figure out why I used OrderedMap for the cooldowns. I say we change it to HashMap and if nothing breaks, keep it

* Move check inherit testing code to separate file * Quickstart: auto-register commands on startup * user_data_setup->setup But keep a deprecated function alias so this is not breaking * Rearrange quickstart type definitions To save one line and to make the ordering more logical (Data and Error always appear in this order in poise, and Context should follow after because it refers to the previous two) * Add register_in_guild and register_globally convenience * Rename listener to event_handler * Fix fmt and docs * Change autocomplete callback value conversion from json::Value::from() to json::to_value() [json!() macro does this]. (#128) * serenity_context and poise::Context traits * Allow supplying timeout to modal execute * Fix example * Fix docs * Bump to 0.5.0 * [Feature] Allow to bypass command checks for owners (#130) * introduce skip_checks_for_owner feature * fix skip_checks_for_owners * fix missing owner check for `skip_checks_for_owners` * Separate cooldowns and min max testing It was stuffed into the add command before, now it's separate commands `cooldowns` and `minmax` * Don't panic on modal timeout * Fallback to HTTP in user_permissions even with cache enabled * Update rustdoc-scrape-examples invocation * Bump to 0.5.1 * Update Cargo.lock To get rid of "cpufeatures 0.2.2 is yanked" message * Impl SlashArgument instead of SlashArgumentHack For primitive types and model types, the slash argumen trait impl is using SlashArgument instead of SlashArgumentHack now, which means those implementations are visible in the docs. Which is very much nicer than before. Also, this commit makes SlashArgument::choices a default method. I decided it's annoying to have to unconditionally implement it even though it's `Vec::new()` in most cases. I originally didn't make it a default method just for "safety" so you don't forget to implement it _if_ you need it but I think that's a bad argument * Fix sneaky bug Welp and I was wondering why editing non-track_edits command `~say` re-runs the command (but not reuses response, or tracks deletions (coming in next commit))... It's because dispatch_message gets given a MessageEditFromInvalid... But why? Yeah because I mixed up the two match cases * Add track_deletion * Make builtins::servers aware of teams. (#133) * Add all Context's methods to Prefix/ApplicationContext Theoretically a small breaking change (some Context methods changed from &self to self. Some lifetimes were also changed from the &self lifetime to 'a, but this is not breaking because the lifetime has been strictly expanded. Anyways I'm not making a breaking release for this because it will realistically affect nobody and it causes more total pain making this a breaking release) * Increase MSRV to 1.60.0 due to time crate And set rust-version Cargo.toml field for the first time (it was added in 1.56) * Update CI badge in README badges/shields#8671 * Bump to 0.5.2 * Add missing `Detailed changelog`'s in CHANGELOG * Add paginate * Always print Command errors to console also The result of a lengthy debugging ssession with Logan King. The problem: if a slash command initial response has an error (e.g. empty content), the interaction completely expires, you can't use it anymore. Any further attempt at sending a response is `Unknown Interaction` or `Unknown Webhook`. So the default error handler for command errors, which just sends the error message to Discord, ran into this, and was unable to send the error message to Discord. And the FrameworkOptions::on_error default error handler emits a log::error in that case, but obv the user doesn't see that when not having logging configured. So Logan King was rightly really confused why their bot was just endlessly showing "... Sending command ..." - that's apparently Discord's helpful way of saying "the bot did respond, but with an error, so we locked this interaction ^.^ and instead of 'this bot responded incorrectly' or something we'll show an endless loading message! this will definitely not confuse anyone ever! ^.^". ANYHOW, this commit unconditionally prints Command errors via eprintln!() as well in the default error handler, so it can't be missed * Feature guard pagination example (#138) * Print login message in framework_usage * Add missing commas in macros for localizations (#143) * Add missing commas in macros for localizations * Add testing code --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> * Document default_member_permissions Fixes #145 * Add EventWrapper example Fixes #146 * Add missing events to EventWrapper (#144) * Add unit test example Fixes #142 * Update links in README.md (kangalioo -> serenity-rs) * Add panic handler (#140) * Restructure dispatch::slash functions like prefix Forgot some stuff from restructguring * Catch panics during command execution - Adds FrameworkError::CommandPanic - The default on_error impl spawns a nice error embed - Wraps all instances of run_invocation/run_interaction/run_autocomplete in catch_unwind - Adds a `div` command to the testing crate to test the panic catching (by triggering a div by zero) * Replace let-else with match to please MSRV * Gate catch_unwind call behind feature flag --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> * Update kangalioo -> serenity-rs and fix typos Changes extracted from the pre-force-push #140 (6c082b5...1bac198) * Split develop branch into current and next * Bump to 0.5.3 * Fix FrameworkContext::user_data being async (#137) * Fix FrameworkContext::user_data being async * fmt * Make FrameworkContext::user_data async again I realized the function was supposed to be async to stay API-compatible with the Framework methods, that's how it was created in c10ec4d. This commit basically reverts #137, except for the lint changes * Optionally display subcommands in help menu Only one recursion level deep so far because infinite recursion would have been a pain and I don't even know if that's what users want so it's single recursion level for now and if someone complains I'll go through the async recursion trouble * Amend to previous commit: clippy allow * Remove redundant import in testing crate * Change CommandPanic payload type to Option<String> Previously, it was Box<dyn Any + Send> - making the entire type not Sync. This wasn't intentional. This is technically a breaking change, but since CommandPanic was only introduced a few days ago, and was itself a breaking change, this is basically just a quick hotfix reversal of a breaking change. * Bump to 0.5.4 * Support #[min_length] #[max_length] Fixes #154 * Bump to 0.5.5 * Make a bunch of structs non_exhaustive... ...and rename AutocompleteChoice.name to label (I think it fits better) * Forgot enums (while applying non_exhaustive) * Test RoleParseError downcasting * Rework of builtins::help() (#161) * Rework of builtins::help() * Use find_command instead of duplicating code * Fix env variable `set` -> `export` for Unix * Fix unneeded mut * cargo fmt --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> * Add HelpConfiguration:include_description #161 changed single help command to include description. This commit makes it optional (but keeps the new behavior as default) * Fix cargo test * Change Command field types - category: `Option<&'static str>` -> `Option<String>` - help_text: `Option<fn() -> String>` -> `Option<String>` - aliases: `&'static [&'static str]` -> `Vec<String>` - context_menu_name: `Option<&'static str>` -> `Option<String>` The &'static references meant you couldn't dynamically change those values which is very powerful so better do this breaking change now than potentially later. It's not like it hurts to use String and Vec here due to the allocations - every bot will have a two-digit, max three-digit number of Command instances anyways and they will all be initialized on startup. Also, the fn() in help_text was really ugly and was just a hack for rustbot to share common parts of the help text. But this should be done by just changing the help_text field in main() or whatever. Actually though, I was able to keep the #[command] attribute's help_text_fn field working. But it runs the function at command construction time now instead of on demand when help is called. So, behavior change technically * Deduplicate CreateReply creation code Adds Context::reply_builder * Remove outdated TODO * Remove pub(super) and make private instead In my opinion pub(crate) or pub(any other path) is a code smell. If you're trying to use it, then either: - your module structure is messed up and you should move code that accesses implementation detail of your type into the same module as that type (this was the reason for the pub(super) from this commit. The required restructuring I apparently already did some time ago) - those "implementation" details should actually be public because trying to fully use that type evidently requires accessing those details (that's why for example poise's EditTracker methods are public, or why the CreateReply::reply_builder method I recently added is public) * Remove unused function warning when testing * Okay apparently Discord needs "es-ES" instead of "es" now * Add ChoiceParameter trait instead of direct impls Fixes #126 * fix: typo (#165) Co-authored-by: xtfdfr <sadorowo@gmail.com> * Support pattern parameters in #[command] Fixes #158 Wow this more complicated than I expected * Fix with simd-json (amend to 8471b6e) * Restructure `#[command]` docs into sub headings * Refinements to builtins::help (#167) * Refinements to builtins::help * Make rustfmt happy * Make MSRV happy --------- Co-authored-by: Rogier 'DocWilco' Mulhuijzen <github@bsdchicks.com> * Add Infallible to __NonExhaustive enum variants * Restructure examples framework_usage and testing are gone in favor of basic_structure and framework_features. The examples/README now describes what each example contains. And the root README points to the examples folder [amend commit] fix rustfmt [amend commit] fix some things to make the example actually runnable (lol), fix feature gates, use poise::builtins::register_globally * Add Context::{cache, http} shorthands And remove the internal `.sc()` function in favor of the `.cache()` shorthand or full blown `.serenity_context()` [amend commit] fix feature gate * Use eprintln in default Setup error handler Previously it used log::error which is not visible enough. If someone doesn't have a logger configured, they will miss it and it will cause BIG TIME confusion. Had it happen to someone I helped debug in #poise, and I Just fell into that trap myself and was downloading and using CodeLLDB, looking through tokio internals, looking up async debuggers, placing dbg!()'s deep within serenity's HTTP code; before finding out it was just some setup error * feature: subcommmand_required parameter (#170) * Add Infallible to __NonExhaustive enum variants * add: subcommmand_required * Fix a few things - The code was not properly formatted (`cargo fmt --all`) - There was a redundant is_subcommand value added to a bunch of things (including find_command - this PR was a breaking change and would have belonged to the `next` branch! But since is_subcommand was redundant and I removed it, this PR becomes not-breaking again (which makes sense; this kind of small feature shouldn't need breaking changes)) - The is_subcommand check was in the wrong place. The parse_invocation is really only for parsing, not for any action. If it returns an error, it should be an error in _parsing_, not in execution or usage. And it also belongs after the track_edits related checks, because if a message was edited that poise isn't even allowed to care about, it shouldn't throw an error too (this would have been another problem with the is_subcommand check in parse_invocation) * Improve text --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> Co-authored-by: xtfdfr <sadorowo@gmail.com> * Make FrameworkError and SlashArgError variants non_exhaustive * Added Support for Opening Modals as an MCI Response (#173) * feat: added support for responding to an mci with a modal * feat: added example, simplified modal send as mci interface * fix: fixed example * Some changes ™ - Move component modal example into modal.rs instead of making new file - Make component modal example prefix_command too to catch any oversights that may make component modals rely on a slash command interaction being present (e.g. previously, execute_component_interaction took ApplicationContext and couldn't be invoked in prefix context at all) - Deduplicate code between execute_modal and execute_component_interaction (and rename the latter to be clearer) - Remove Modal::execute_component_interaction function in favor of free-standing execute_modal_on_component_interaction function - I want to avoid an explosion of utility trait functions for any combination of defaults yes/no, component yes/no... That said, I realize we're already halfway in this mess with execute and execute_with_defaults both existing so I'd also be open to go all the way and add execute_component_interaction and execute_component_interaction_with_defaults - Add `impl AsRef<serenity::Context> for poise::Context` to make free-standing execute_modal_on_component_interaction function work well; and because it makes sense in general to have --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> * Add CooldownContext struct to make Cooldowns usable in more situations (#175) * Convert Cooldowns to use HashMap instead of OrderedMap for better scaling (#180) * Fix failing CI build (#181) - Update serenity 0.11.6 -> 0.11.5 - Update proc_macro2 1.0.47 -> 1.0.64 - Disambiguate several serenity re-exports - Temporarily disable serenity/simdjson test since feature is broken upstream (serenity-rs/serenity#2474) * add `reply()` for `poise::context` (#183) * fix the behavior of poise::reply::builder::CreateReply.reply * unnecessary change by mistake * change back to original * add reply() * remove reply_reply() * Adhere to unwritten codebase conventions Made on phone in GitHub's absolutely janky web UI, and untested. Let's see how this goes --------- Co-authored-by: kangalio <jannik.a.schaper@web.de> * Two dependency bumps (#186) * Bump `tokio` to `1.25.1` * Bump `env_logger` to `1.10.0` * Bump `tokio` to `1.25.1` * Allow passing cooldown config on demand That way, you can have different cooldown durations depending on which user / guild / whatever is invoking the command * Remove old `CooldownTracker` methods and migrate internals - Renames `CooldownTracker::new_2` and `CooldownTracker::remaining_cooldown_2` to `CooldownTracker::new` and `CooldownTracker::remaining_cooldown` respectively. - Adds a new `cooldown_config` field to `Command` wrapped in a Mutex to allow updating by the user - Adds a new example using a command check to modify the `cooldown_config` on the `Command` - updates `#[poise::command]` to new `Command` structure * Bump MSRV to 1.63 Needed for dependency updates in #186 that were done to resolve some `cargo audit` vulnerabilities * Remove uneccessary NonZeroU64 usage The new serenity id types impl PartialEq<u64> and From<u64> so we don't need to bother with hhe NonZeroU64 directly * Fix copy paste typo in pagination button IDs Co-authored-by: jamesbt365 <jamesbt365@gmail.com> * Fix empty initial message in pagination Co-authored-by: jamesbt365 <jamesbt365@gmail.com> --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> Co-authored-by: ChancedRigor <chancedrigor@users.noreply.github.com> Co-authored-by: Peanutbother <6437182+peanutbother@users.noreply.github.com> Co-authored-by: Andre Julius <noromoron@gmail.com> Co-authored-by: Gnome! <45660393+GnomedDev@users.noreply.github.com> Co-authored-by: Maximilian Mader <max@bastelstu.be> Co-authored-by: Whitney <67877488+whitbur@users.noreply.github.com> Co-authored-by: Horu <73709188+HigherOrderLogic@users.noreply.github.com> Co-authored-by: docwilco <66911096+docwilco@users.noreply.github.com> Co-authored-by: franuś <97941280+xtfdfr@users.noreply.github.com> Co-authored-by: xtfdfr <sadorowo@gmail.com> Co-authored-by: Rogier 'DocWilco' Mulhuijzen <github@bsdchicks.com> Co-authored-by: G0ldenSp00n <71467406+G0ldenSp00n@users.noreply.github.com> Co-authored-by: B-2U <82710122+B-2U@users.noreply.github.com> Co-authored-by: Overzealous Lotus <103554783+OverzealousLotus@users.noreply.github.com> Co-authored-by: jamesbt365 <jamesbt365@gmail.com>

serenity-rs#175)

* Optionally display subcommands in help menu Only one recursion level deep so far because infinite recursion would have been a pain and I don't even know if that's what users want so it's single recursion level for now and if someone complains I'll go through the async recursion trouble * Make a bunch of structs non_exhaustive... ...and rename AutocompleteChoice.name to label (I think it fits better) * Forgot enums (while applying non_exhaustive) * Rework of builtins::help() (#161) * Rework of builtins::help() * Use find_command instead of duplicating code * Fix env variable `set` -> `export` for Unix * Fix unneeded mut * cargo fmt --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> * Add HelpConfiguration:include_description #161 changed single help command to include description. This commit makes it optional (but keeps the new behavior as default) * Fix cargo test * Change Command field types - category: `Option<&'static str>` -> `Option<String>` - help_text: `Option<fn() -> String>` -> `Option<String>` - aliases: `&'static [&'static str]` -> `Vec<String>` - context_menu_name: `Option<&'static str>` -> `Option<String>` The &'static references meant you couldn't dynamically change those values which is very powerful so better do this breaking change now than potentially later. It's not like it hurts to use String and Vec here due to the allocations - every bot will have a two-digit, max three-digit number of Command instances anyways and they will all be initialized on startup. Also, the fn() in help_text was really ugly and was just a hack for rustbot to share common parts of the help text. But this should be done by just changing the help_text field in main() or whatever. Actually though, I was able to keep the #[command] attribute's help_text_fn field working. But it runs the function at command construction time now instead of on demand when help is called. So, behavior change technically * Add ChoiceParameter trait instead of direct impls Fixes #126 * Fix with simd-json (amend to 8471b6e) * Refinements to builtins::help (#167) * Refinements to builtins::help * Make rustfmt happy * Make MSRV happy --------- Co-authored-by: Rogier 'DocWilco' Mulhuijzen <github@bsdchicks.com> * Make FrameworkError and SlashArgError variants non_exhaustive * Migrate cooldowns to new version * Add CooldownContext struct to make Cooldowns usable in more situations (#175) * Convert Cooldowns to use HashMap instead of OrderedMap for better scaling (#180) * Convert poise builders to new serenity style * Swap to serenity 0.12.0 * Bump MSRV to match serenity's --------- Co-authored-by: kangalioo <jannik.a.schaper@web.de> Co-authored-by: docwilco <66911096+docwilco@users.noreply.github.com> Co-authored-by: Rogier 'DocWilco' Mulhuijzen <github@bsdchicks.com> Co-authored-by: scottbot95 <scottbot95@gmail.com>

kangalio reviewed Jul 9, 2023

View reviewed changes

scottbot95 force-pushed the current branch from abb4762 to 880451d Compare July 9, 2023 18:29

scottbot95 changed the base branch from current to next July 9, 2023 18:29

Add CooldownContext struct to make Cooldowns usable in more situations

c905280

scottbot95 force-pushed the current branch from 880451d to c905280 Compare July 9, 2023 23:08

kangalio merged commit 6cc521a into serenity-rs:next Jul 10, 2023

kangalio mentioned this pull request Jul 10, 2023

Change cooldowns' OrderedMap to HashMap #179

Closed

This was referenced Jul 10, 2023

Convert Cooldowns to use HashMap instead of OrderedMap for better scaling #180

Merged

Allow dynamic CooldownConfig #185

Closed

GnomedDev pushed a commit to GnomedDev/poise that referenced this pull request Nov 22, 2023

Add CooldownContext struct to make Cooldowns usable in more situations (

834c12a

serenity-rs#175)

GnomedDev pushed a commit to GnomedDev/poise that referenced this pull request Nov 26, 2023

Add CooldownContext struct to make Cooldowns usable in more situations (

bd73861

serenity-rs#175)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CooldownContext struct to make Cooldowns usable in more situations #175

Add CooldownContext struct to make Cooldowns usable in more situations #175

scottbot95 commented Jul 9, 2023

kangalio left a comment

scottbot95 commented Jul 9, 2023

scottbot95 commented Jul 9, 2023

kangalio commented Jul 9, 2023

scottbot95 commented Jul 9, 2023 •

edited

Loading

scottbot95 commented Jul 9, 2023

kangalio commented Jul 10, 2023

Add CooldownContext struct to make Cooldowns usable in more situations #175

Add CooldownContext struct to make Cooldowns usable in more situations #175

Conversation

scottbot95 commented Jul 9, 2023

Problem

Proposal

kangalio left a comment

Choose a reason for hiding this comment

scottbot95 commented Jul 9, 2023

scottbot95 commented Jul 9, 2023

kangalio commented Jul 9, 2023

scottbot95 commented Jul 9, 2023 • edited Loading

scottbot95 commented Jul 9, 2023

kangalio commented Jul 10, 2023

scottbot95 commented Jul 9, 2023 •

edited

Loading