Allow hubot to react on events #344

Merged
merged 8 commits into from Feb 16, 2013

Projects

None yet

5 participants

@nesQuick
Contributor

Events might get triggered by scripts.

For example: You have a script, which can trigger and event that some one committed to a repo. This script can be used by anyone, but the reaction should be different by every company. So it's possible to trigger a build, or just post a message in chat.

As a plus with the same event triggered by different scripts, so it wouldn't make a difference if you commit into github or just another (maybe SVN) repo. But other scripts can rely on that 'commit-received' command and act the same way.

@nesQuick nesQuick Allow hubot to react on events
Events might get triggered by scripts.

For example: You have a script, which can trigger and event that some one committed to a repo. This script can be used by anyone, but the reaction should be different by every company. So it's possible to trigger a build, or just post a message in chat.

As a plus with the same event triggered by different scripts, so it wouldn't make a difference if you commit into github or just another (maybe SVN) repo. But other scripts can rely on that 'commit-received' command and act the same way.
2b308fe
@technicalpickles
Member

This looks interesting, but I'd like to see some docs and some usage examples before this gets merged.

@nesQuick
Contributor

Good point, i'll add docs. Any wishes where to add?

And examples? in code or would you like to see them in docs. b/c at the included script there are IMO no real usecase for that.

@technicalpickles
Member

README is the logical place. Doesn't look like there's any API changes, but maybe it'd be worthwhile to add some convenience methods to have a point to be documenting.

I wouldn't force them into the included scripts if it doesn't apply. But, maybe a new simple script that it would be useful for?

On Aug 28, 2012, at 8:54 AM, Ole Michaelis wrote:

Good point, i'll add docs. Any wishes where to add?

And examples? in code or would you like to see them in docs. b/c at the included script there are IMO no real usecase for that.


Reply to this email directly or view it on GitHub.

@nesQuick
Contributor

This is only README.md next is to find a suitable solution for an example, beside that mentioned in README or might this be already enough?

@ferlores
Contributor

+1

@tombell
Contributor
tombell commented Sep 24, 2012

This would be better if you just extended the Robot class with EventEmitter.

class Robot extends EventEmitter

Then you can have:

robot.on 'something', ->
  # ...

Rather than an ugly events property.

@nesQuick
Contributor

Extending would break all kind of code semantics. If you prefere the on method call, we should call the event emitter inside this method.

@technicalpickles
Member

I agree with @nesQuick. The robot.on 'something' syntax is nice though. Maybe we could expose that same API, but have an instance of an event emitter? That is, use composition instead of inheritance.

@tombell
Contributor
tombell commented Oct 27, 2012

The Robot instance is what is listening and reacting to events. So I don't get how you think it's breaking code semantics.

@nesQuick
Contributor

So the best example why not use inheritance is that if we some day need another functionality which can be archived by inheritance, there is no multiple inheritance.
So composition would be the better solution. I'll change it the next days :)

@tombell
Contributor
tombell commented Jan 10, 2013

Closing this, it can lead to a lot of weird issues with different scripts listening for different events which may not be compatible with each other.

@tombell tombell closed this Jan 10, 2013
@technicalpickles
Member

I'm thinking this could be a nice feature for v3.0.

You say it could weird issues with different scripts listening for different events, but hubot has that problem with different scripts listening for different text. The best way to solve that is with documentation, I think. Scripts that emit events should document that, like we do dependencies, configs, etc. Same for scripts that consume the events.

The use case I have in mind is for making scripts that consume some service, and allow extension by sending these events. For example, I have a private script that checks a PagerDuty schedule, and texts as a person as they come on call, and also notifies a Campfire room. Because of that, I can't really open source it... but if I have it emit something like pagerduty.shiftchange, I could open source that script, and as cript to consume the event and post to Campfire, then have a private script for the text notification.

@tombell
Contributor
tombell commented Jan 13, 2013

This can easily just be added to hubot with a script, I don't think complicating hubot more is a good direction.

{EventEmitter} = require 'events'
module.exports = (robot) ->
  robot.events or= new EventEmitter
  # do shit
@technicalpickles
Member

That code snippet, particularly of setting robot.events, seems like it'd be open to the type of 'weird issues' you were talking about.

But thinking about it, if you are in a pagerduty script, and you set robot.pagerdutyEvents to a new emitter, that might not be the worst.

@tombell
Contributor
tombell commented Jan 14, 2013

My code snippet functions exactly like the code that would be added to hubot itself, if every script wanting to use events included that at the top it would work fine.

The or= makes sure the robot.events is never re-initialised.

@nesQuick
Contributor

I was in the process of improving this PR. I would love to see it in v3.0 please give me some more weeks to improve it.

I also really like the way @technicalpickles described, this was also my initial attempt to this PR. So I think we're on the same way having such a feature. Is there a milestone or roadmap for 3.0? I would love to help also with other tickets.

Tom Bell and others added some commits Feb 8, 2013
Tom Bell Merge pull request #408 from pgib/at-reply-detection
Support @<name> convention in the Robot's respond listener
f988056
@nesQuick nesQuick Allow hubot to react on events
Events might get triggered by scripts.

For example: You have a script, which can trigger and event that some one committed to a repo. This script can be used by anyone, but the reaction should be different by every company. So it's possible to trigger a build, or just post a message in chat.

As a plus with the same event triggered by different scripts, so it wouldn't make a difference if you commit into github or just another (maybe SVN) repo. But other scripts can rely on that 'commit-received' command and act the same way.
7af8295
@nesQuick nesQuick added docs for event system 23ff672
@nesQuick nesQuick examples and fixed documentation for event system a060466
@nesQuick nesQuick Merge branch 'events' of github.com:nesQuick/hubot into events
Conflicts:
	README.md
	src/robot.coffee
ced8181
@nesQuick
Contributor

Oh shit - long time ago since i did the last pull/merge/rebase.

But I finally managed to complete this. I hope the semantics now matches the expectations and documentation and examples are verbose enough 😃

❤️

@tombell tombell and 1 other commented on an outdated diff Feb 12, 2013
src/robot.coffee
+ # event - The event name.
+ # listener - A Function that is called with the event parameter
+ # when event happens.
+ #
+ # Returns nothing.
+ on: (args...) ->
+ @events.on args...
+
+ # Public: A wrapper around the EventEmitter API to make usage
+ # semanticly better.
+ #
+ # event - The event name.
+ # [arg1] - Event argument one (optional).
+ # [arg2] - Event argument two (optional).
+ # [....] - Event argument n (optional).
+ #
@tombell
tombell Feb 12, 2013 Contributor

This should just be args - An Array of arguments emitted by the event.

@nesQuick
nesQuick Feb 12, 2013 Contributor

But that's not an array, it's a bunch of arguments. First argument is always treated as the event name, so if you would pass an array, the event name would be the string representation of that array which isn't complaint to the event emitter interface.

@tombell
tombell Feb 14, 2013 Contributor

What is the reason for using (args...) instead of (event, args...) which is more understandable?

@nesQuick
nesQuick Feb 14, 2013 Contributor

fixed :) also applied the changes to the on method to make it more consistent

@nesQuick nesQuick Updated method headers
to make them more understandable. see github#344 (comment)
936acd7
@tombell tombell merged commit 41741ca into github:master Feb 16, 2013
@rdohms
rdohms commented Feb 22, 2013

I'm getting an error which seems to track back to this PR:

ERROR Unable to load /scripts/events: TypeError: Object #<Robot> has no method 'on'
TypeError: Object #<Robot> has no method 'on'
    at module.exports (/data/home/webclusive/bots/r2d2/scripts/events.coffee:13:18)
    at Robot.loadFile (/data/home/webclusive/bots/r2d2/node_modules/hubot/src/robot.coffee:122:24)
    at Robot.load (/data/home/webclusive/bots/r2d2/node_modules/hubot/src/robot.coffee:140:33)
    at Object.fs.exists [as oncomplete] (fs.js:91:19)

Am I missing a library or something?

@tombell
Contributor
tombell commented Feb 22, 2013

a release of hubot containing this PR hasn't been released.

@nesQuick
Contributor

@rdohms can you provide more details pls? I cant reproduce it

@rdohms
rdohms commented Feb 22, 2013

@nesQuick its just a regular checkout of hubot with the regular stuff on the unix how-to. I checked out version 2.4.6 and no more problems.

@nesQuick
Contributor

Oh yeah, took a while until I get the problem, sorry. I think it's ok until we'll make a release 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment