How to properly reimplement base hooks?

[slack-ruby]

How to properly reimplement base hooks?

[dramalho]

For now I'm doing this in my initializer

    def initialize(options = {})
      self.class.hooks.uniq!  # Dedup hooks

      super
    end

but it's mostly hacky :)

[dblock]

I think the way I implemented this is really not very smart ;) If you look at https://github.com/dblock/slack-ruby-bot/blob/7f54f00cbcd17ff4fba0a3c95de332f9e81ec4cd/lib/slack-ruby-bot/server.rb#L96 we assume a lot of things here. First that the class matches the method, which makes it impossible to include multiple things like the example you give. I say we fix it properly.

I would either kill SlackRubyBot::Hooks::Base altogether or just deprecate that way of doing things in favor of something much more elegant:

class MyBotServer < Server
  on :hello do
     # evaluates in the context of this class
  end
end

I did a similar refactor in the client here around events and it turned out to be a lot cleaner.

Want to give it a shot?

[dramalho]

So, to be honest, I like both syntactic sugar approach (for small enough bots) and the one-class per hook implementation (like the commands) for more complex stuff, it helps keep the code decent sized so, again, like the commands, I would rather have both options. Also, I don't see a particular reason why two different handlers couldn't be notified of the same event, so I would put that in.

I also understand that at least for the message hook, it's great to have a default implementation there out of the box, but it probably shouldn't stop some happy person from rolling out it's own - without re-implementing ::Server - so maybe add a way to unregister / clean hooks.

re: magic / assumptions, it can be made a bit sane I guess, descendants of Base, class name matches hook name, we can go either way, magic (class exists) or explicit (someone registers a hook class) and then each class can have a common interface (i.e. hook classes receive call with a bunch of params)

Sorry, I'm just rambling a bit on the train :) , but I would roughly enjoy having:

• Multiple handlers per hook
• Explicit way to remove handlers (say the default ones)
• Syntactic command-like sugar for simpler single file bots, but also individual handler classes
• Still be able to just extend Server / App and tailor configurations

And yeah, I don't mind taking a stab at this, but it's your baby, I want to make sure I move in a direction you agree with :) , so what do you think?

[dblock]

Everything you say makes total sense. Would love to see you contribute the missing parts!

Multiple handlers per hook

Definitely. A hook is just a callback mechanism, so being able to do multiple things in it would be great.

Explicit way to remove handlers (say the default ones)

I think having the default add them and client code remove them is hacky, I would rather add a way not to add the default ones via some configure or similar call.

Syntactic command-like sugar for simpler single file bots, but also individual handler classes

Yes.

Still be able to just extend Server / App and tailor configurations

👍

[dramalho]

| I think having the default add them and client code remove them is hacky, I would rather add a way not to add the default ones via some configure or similar call.

yeah, I'm just thinking of the scenario where this is there from the start and you can actually get a basic bot working out of the box.

I'll have a think about that one in particular, agree that undoing something super did feels hacky

[dramalho]

I'll carve some time to work on this, if I can I'll break it into separate PR's but I need to structure it first :)

like I said, happy to help :)

[dramalho]

actually, happy to give something back in return for the work that you did :) is more correct

[dblock]

👍

[dramalho]

Naive implementation #1 dblock/slack-ruby-bot@master...dramalho:hook-improvements, on my bot I can then configure handlers like

{
        hello: [
          SlackRubyBot::Hooks::Hello.new(logger),
          Bot::Events::Hello.new
        ],
        message: SlackRubyBot::Hooks::Message.new(logger),
        user_presence: Bot::Events::UserChange.new,
        presence_change: Bot::Events::PresenceChange.new
      }

mehish

[dblock]

Other than some naming suggestions I think that's actually an improvement, I would turn this into a PR.

[dramalho]

I'll throw specs at it. Naming suggestions welcome, it's the bane of my
existence

On Fri, 4 Mar 2016, 13:16 Daniel Doubrovkine (dB.) @dblockdotorg, <
[email protected]> wrote:

Other than some naming suggestions I think that's actually an improvement,
I would turn this into a PR.

—
Reply to this email directly or view it on GitHub
https://github.com/dblock/slack-ruby-bot/issues/50#issuecomment-192280491
.

[dblock]

I only have caching suggestions :)

Generally I think you should keep names short. For example, register_handler would belong in a class called Registry::Handlers instead of just Registry, and then the method becomes register or just add, and hook_name becomes just name. That's just to illustrate, not sure whether that's right here.

[dramalho]

Yes, no one will ever know David for it's ability to write short names :D . register sounds cool, the context is narrow anyway, so people won't get confused by what they're registering.

Perfect, I'll work on adding specs and clean this up .

There's one annoying factor though, people who upgrade to this will potentially have their hook handlers break on them. Not sure if I can make it fully backwards compatible (I can try)

[dblock]

I would try, but not more than that, for backwards compatibility. Few people do custom hooks, and we have a clear https://github.com/dblock/slack-ruby-bot/blob/master/UPGRADING.md document that should explain what's going on. This stuff is pretty new, we can break things for now.

[dramalho]

Quick update, just so you don't think I disappeared :)

I obviously broke every spec in the system, so I changed the rspec matcher a bit to use the Message hook directly - although I think there's a bit too much rolling on that message Hook, or at least I would like specs to me more explicit about it, I won't do much more refactoring now .
I need to tackle the block like commands too, but given they are Procs and I can call them, it should just be a matter of simply adding them to the registry, but I haven't done that yet.

And then my own specs :)

[dblock]

👍

[dblock]

It's looking good. Something still bugs me about the name Registry, it's very Windows. What do you think of just Set?

[dramalho]

Hooks::Set is fine and dandy :P , I'll change that up

[dramalho]

and yeah, it's been yeeeeeeeeeeeeears since I last used Windows, but I get what you mean, I get exactly what you mean :P

[dblock]

[dramalho]

OMG now I want to write a ICQ bot to run on my windows 95 box :D

[dramalho]

Cool, specs are passing and all that jazz, I'll open a PR and we can more the discussion there :)

[dramalho]

Closed by the power of #54