New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow hubot to react on events #344

Merged
merged 8 commits into from Feb 16, 2013

Conversation

Projects
None yet
5 participants
@OleMchls
Copy link
Contributor

OleMchls commented Aug 28, 2012

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.

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.
@technicalpickles

This comment has been minimized.

Copy link
Contributor

technicalpickles commented Aug 28, 2012

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

@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Aug 28, 2012

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

This comment has been minimized.

Copy link
Contributor

technicalpickles commented Aug 28, 2012

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.

@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Aug 28, 2012

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

This comment has been minimized.

Copy link
Contributor

ferlores commented Aug 28, 2012

+1

@tombell

This comment has been minimized.

Copy link
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.

@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Oct 14, 2012

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

This comment has been minimized.

Copy link
Contributor

technicalpickles commented Oct 27, 2012

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

This comment has been minimized.

Copy link
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.

@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Oct 28, 2012

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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Contributor

technicalpickles commented Jan 13, 2013

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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Contributor

technicalpickles commented Jan 13, 2013

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

This comment has been minimized.

Copy link
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.

@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Jan 14, 2013

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
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.
@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Feb 11, 2013

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 😃

❤️

# [arg1] - Event argument one (optional).
# [arg2] - Event argument two (optional).
# [....] - Event argument n (optional).
#

This comment has been minimized.

@tombell

tombell Feb 12, 2013

Contributor

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

This comment has been minimized.

@OleMchls

OleMchls Feb 12, 2013

Author 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.

This comment has been minimized.

@tombell

tombell Feb 14, 2013

Contributor

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

This comment has been minimized.

@OleMchls

OleMchls Feb 14, 2013

Author Contributor

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

Updated method headers
to make them more understandable. see #344 (comment)

tombell pushed a commit that referenced this pull request Feb 16, 2013

Tom Bell
Merge pull request #344 from nesQuick/events
Allow hubot to react on events

@tombell tombell merged commit 41741ca into hubotio:master Feb 16, 2013

@rdohms

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

tombell commented Feb 22, 2013

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

@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Feb 22, 2013

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

@rdohms

This comment has been minimized.

Copy link

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.

@OleMchls

This comment has been minimized.

Copy link
Contributor Author

OleMchls commented Feb 23, 2013

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