search

serenity-rs / poise

Discord bot command framework for serenity, with advanced features like edit tracking and flexible argument parsing
MIT License
630 stars 112 forks source link

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

Closed scottbot95 closed 1 year ago

scottbot95 commented 1 year ago

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.

scottbot95 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thank you for implementing my requests!

Do you have opinions on implementing From? 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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