Skip to content
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

Add an 'ignore' method to the robot #686

Closed
wants to merge 5 commits into from
Closed

Conversation

dancrumb
Copy link

I'd like my bot to be able to stop listening and hearing things.

Thus, I'd like an 'ignore' method, or an ability to get something back from hear and listen that I can then call stop on, to stop hearing and responding.

@dancrumb
Copy link
Author

I will make sure that order of listeners is maintained, so that marking a message as done works as before

@technicalpickles
Copy link
Member

Are you talking about being able to disable specific listeners, or just stopping receiving all messages?

There is an array of listeners on the robot instance, so feasibly could manipulate that. I could also imagine making adjustments to for similar usage to setTimeout and setInterval, such that hear and respond return something that can then be unheard or unresponded to.

@dancrumb
Copy link
Author

Oh yeah... I'm working on a fix for this right now :)

@dancrumb
Copy link
Author

Specific listeners.

I envision something like

// start listening
var handle = robot.listen(/blah/, function (msg) {···});


//...


// stop listening
handle.stop();

@dancrumb
Copy link
Author

A one-and-done would be

var handle = robot.hear(regex, function (m) {
    //...

    handle.stop();

});

@technicalpickles
Copy link
Member

I'm not sure that particular would work, since the reference to handle wouldn't be defined in the function. I think it'd have to be passed into the function.

On April 16, 2014 at 6:44:54 PM EDT, Dan Rumney notifications@github.com wrote:A one-and-done would be var handle = robot.hear(regex, function (m) { //... handle.stop(); }); —Reply to this email directly or view it on GitHub.

@dancrumb
Copy link
Author

It will. The callback forms a closure which includes handle. By the time the callback is invoked, handle will have a value.

Here's an example using setTimeout: http://jsfiddle.net/94g2k/2/

@dancrumb
Copy link
Author

Pull request created

@technicalpickles
Copy link
Member

Did you mean to close it?

On April 17, 2014 at 1:32:36 PM EDT, Dan Rumney notifications@github.com wrote:Pull request created —Reply to this email directly or view it on GitHub.

@dancrumb
Copy link
Author

I did not. Didn't realise I had. The PR is ready to go.

@dancrumb
Copy link
Author

Oh... the PR i originally opened, I closed, because this issue is now a Pull Request

@dancrumb
Copy link
Author

dancrumb commented May 1, 2014

nudge
Anything I can do to help move this PR along? I'd love to get back to using this repo, instead of my fork, if that's possible.


# WebStorm files
/.idea
atlassian-ide-plugin.xml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add these? Is this specific to your environment? If so, can they be removed for the purposes of this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are a directory and a file generated by JetBrain's WebStorm IDE.

I can certainly remove them, if you prefer, but I'd advocate for leaving
them in. They're commented, they do no harm and they make it easier for
other WebStorm users to provide code.

I'm comfortable with either outcome.

Confirm which you'd like and I'll make any necessary changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best way to handle these sorts of files is to create a global gitignore file on your machine. This way you can ignore all your system-specific files without needing to change individual project's gitignores.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as requested.

Thanks for the tip, @iangreenleaf

@michaelansel
Copy link
Collaborator

I just submitted a PR (#724) for a Listener hooks framework that may make this easier. To stop accepting all messages, you would just create a hook that always fails.

@michaelansel
Copy link
Collaborator

Taking back my previous comment in favor of pointing to #778

@michaelansel
Copy link
Collaborator

More cross-linking; listener de-registration could be a useful construct to include in a new script interface (#858).

I like the idea of robot.hear (and friends) returning a usable handle, though I wonder if we can make that the Listener object instead of a proxy.

Also, I wonder if this could be implemented with middleware: retrieve a listener by ID and then set metadata to "disabled", which a piece of listener middleware would then use as an indicator to fail.

Rough idea for middleware:

The middleware:

module.exports = (robot) ->
  robot.listenerMiddleware (robot, listener, response, next, done) ->
    if listener.options.ignore?
      # Block listener execution
      done()
    else
      # Continue
      next(done)

A script that toggles itself on and off:

module.exports = (robot) ->
  robot.hear /message123/, id: 'my-cool-listener', (response) ->
    # time to turn off
    robot.listenerById('my-cool-listener').options.ignore = true
    # ....
    # time to turn back on
    robot.listenerById('my-cool-listener').options.ignore = false

Depends on #803 and robot.listenerById (#823 (comment))

@michaelansel
Copy link
Collaborator

Opened #1031 to add robot.listenerById. When that merges, I'll close this.

@bkeepers
Copy link
Contributor

Since it looks like #1031 is the preferred way of handling this, I'm going to go ahead and close this. Thanks for your contributions!

@bkeepers bkeepers closed this May 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants