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 bot-initiated messaging ability #95

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

csaftoiu
Copy link

The approach is to use an idle handler, which gets called whenever something else hasn't already happened, or once per second. This should help reduce rate limiting.

Functional testing has been added.

Updated README.md with how to use the new functionality.

Resolves #92.

Dev Notes

To allow the idle handler to send messages, I give it the instance of the SlackClient as a parameter.

To make SlackClient easier to use, I made send_message, rtm_send_message, and upload_file all take a "channelish" string, instead of a channel. This turns a string into a channel as follows:

  • Given a channel id, return that same channel id.
  • Given a channel name, return the channel id.
  • Given a user id, return the direct message channel with that user, opening a new one if necessary.
  • Given a user name, do the same as for a user id.

This way a user can just client.send_message("some_user", "Here the message") instead of having to call any other helper functions.

The approach is to use an idle handler, which gets
called whenever something else hasn't already
happened, or once per second. This should help
reduce rate limiting.

Functional testing has been added.

Updated README.md with how to use the new
functionality.

Resolves scrapinghub#92.
@csaftoiu
Copy link
Author

Oh boy, didn't see #89 and #54 already. Hmm well I'll leave this open as it's another approach. It's basically like #54 except it provides easy access to the client right there, and a nicer interface for sending messages (using channel names & usernames instead of ids).

Also it opens direct message channels if they aren't open yet. I think this may be necessary for messaging a user who has never messaged you, for example.

@smrpr
Copy link

smrpr commented Jul 21, 2016

@lins05 due to @prathyush not updating his PR, do you think we could focus on this one and fix what is needed to merge it? I think this would be really useful for slackbot.

@@ -72,3 +73,6 @@ def get_plugins(self, category, text):

if not has_matching_plugin:
yield None, None

def get_idle_plugins(self):
yield from self.idle_commands

Choose a reason for hiding this comment

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

@csaftoiu: Not compatible with Python2.7. Can be changed to the following -
for c in self.idle_commands: yield c

Copy link
Collaborator

@jtatum jtatum left a comment

Choose a reason for hiding this comment

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

This is a neat PR. One of the big concepts it introduces is the idea of passing the client directly to a function. In the past, the client has been in message._client, implying that it's not meant to be a public interface. The changes you had to add to client (like channelfy and so on) reflect that. So, either we can clean up client and say that it has a stable API and should be user facing, or we should make a new type of interface, like a client-type object, that we expose both in messages and idle handlers. I think @lins05 might be a better person to make that decision 😄

@idle
def bored(client):
if time.time() - last_bored >= 30:
last_bored = time.time()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need global last_bored to write to the outer scoped var?

@@ -65,6 +66,17 @@ def wrapper(func):
return wrapper


# use optional_arg_decorator so users can either do @idle or @idle()
@optional_arg_decorator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain the purpose of supporting @idle or @idle()?

if self._client.idle_time() < 1:
break

try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if the next few lines should be in loop() or elsewhere. Unlike regular messages, this would mean that idle functions are run in the loop thread. Other messages are handled in their own thread, being added to the pool.

@olliencc
Copy link

sad this isn't merged.. this is a killer PR

@mew1033
Copy link

mew1033 commented Sep 14, 2017

Looks awesome! I'm excited to see this merged!

@nspo
Copy link

nspo commented Mar 19, 2018

Any chance of this being merged?

@oannam
Copy link

oannam commented May 24, 2018

Is there any plan for this PR to be merged? I've seen that there are requests for changes. @csaftoiu any thoughts? In a fork of this repo I managed to build on top of the functionality added by @csaftoiu and added the possibility of running a function at certain times.

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.

8 participants