search

rikai / Showbot

🤖 An omnipresent multi-platform bot who's goal in life is become Skynet 🤖
MIT License
40 stars 18 forks source link

Implement command cooldowns using cinch-cooldown #40

Closed rikai closed 7 years ago

rikai commented 8 years ago

This is something i'd like implemented before the next release, probably the highest priority thing in the queue...

The blocker issue is described here

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

s0ph0s-2 commented 8 years ago

I've narrowed down the issue to this function. The middle argument, shared, seems to be expecting something other than what it's getting, which causes the unless on line 18 to fail, stopping it from triggering. More work to come…

s0ph0s-2 commented 8 years ago

It turns out that some part of Showbot is changing the YAML interpreter's behavior to always use symbols for keys. As such, the channel names in the YAML config become :"#channel", not just "#channel", so the cinch-cooldown plugin thinks the configuration file is broken and doesn't do anything. A workaround function that loops through the hash and converts the channel keys back to strings does properly make it limit command use.
Tomorrow, I'll either figure out how to either undo the thing that's breaking the YAML behavior, or submit a change upstream that papers over it.

s0ph0s-2 commented 8 years ago

This function is being called recursively on the config Hash, which converts all keys to symbols for faster access. Unfortunately, this wrecks cinch-cooldown's configuration settings, which depend on string keys.

s0ph0s-2 commented 8 years ago

PR submitted, pending approval.

s0ph0s-2 commented 8 years ago

bhaberer has expressed a concern that converting the keys back to strings if Cinchize is loaded is kind of a crufty way of fixing it. He is considering updating cinch-cooldown to 1.2.0 and expecting symbols internally.

rikai commented 8 years ago

@s0ph0s-2 Seems that cinch-cooldown has been updated with the changes we need, hopefully should be smooth sailing now! :+1:

s0ph0s-2 commented 8 years ago

Great!

rikai commented 7 years ago

@s0ph0s-2 Hoping to get this knocked out sometime soon if you get a chance to look at it. :)

s0ph0s-2 commented 7 years ago

I took a look during a hackathon in November, but I wasn't able to get it quite finished. I'm in the thick of the spring semester right now, though, so I won't be able to finish it until the end of May.

s0ph0s-2 commented 7 years ago

This is bizarre. I don't know what magic I was doing earlier to get it to rate-limit properly — it certainly isn't doing it now.
The monkey-patched hook that cinch-cooldown runs before each message is running, but the function it calls to determine if a cooldown needs to be applied doesn't seem to be. I can't tell whether that's because I'm loading the plugin in a non-standard way (for testing), or if it's because cinchize is breaking something.
Regardless, I'll keep you posted as I work on this.

s0ph0s-2 commented 7 years ago

Aha! I've found the problem. cinch-cooldown was never updated on rubygems.org, so the fix was never propagated.
Relevant issue on cinch-cooldown.

s0ph0s-2 commented 7 years ago

Still waiting on @ bhaberer to update the version on rubygems.org.

rikai commented 7 years ago

No worries, looks like he plans to get to it tonight!

s0ph0s-2 commented 7 years ago

Okay, the changes have been pushed to rubygems.org. I've tested, and things seem to be working properly.

Now, there are two more questions:

  1. What plugins should have cool downs enabled? All of them?
  2. What should the cool down values be?
s0ph0s-2 commented 7 years ago

Relevant IRC conversation:

22:33 <~rikai> s0ph0s: if i'm being honest, i'm not sure in respsnse to your questions... I think that most plugins, perhaps with the exception of !suggest, should have cooldowns. Suggest has a high rate of usage at some points, so i'm not sure it should have a cooldown, and it isn't tha spammy... i imagine most others probably need it though...
22:35 <s0ph0s> Okay. Should I guess at the cooldown times from the channel modes then? As with the other anti-spam measure, we can always tweak the numbers if they're not working.
22:39 <~rikai> Guess a best guess is better than nothing, can always tweak from there
22:39 <s0ph0s> Okay then! I'll stick the two lines necessary to enable cinch-cooldown into all of the relevant plugins.
s0ph0s-2 commented 7 years ago

I've omitted both suggest.rb and admin.rb from the cooldowns, since both of those should respond ASAP.