Handle empty messages in bot framework by default #58

neiljp · 2017-08-12T05:10:27Z

This aims to implement #55.

These commits:

Modify lib.py to intercept empty messages, if the bot class does not explicitly define ACCEPT_EMPTY_MESSAGES (and evaluates to True)
Amend a local doc to mention this optional element of bot classes
Amend the wikipedia bot, as a trial bot.

Outstanding points regarding this level of implementation:

Other than the wikipedia bot, all other bots respond to empty messages via lib.py, since they've not all been updated.
All the tests pass! This is since the tests just test the bot code, and so aren't aware of this interception.
Is this document the accurate bot document? What about docs/bot-guide.md in the main repo?
Is ACCEPT_EMPTY_MESSAGES a typical name/format to use in this case?

roberthoenig

Thanks for working on this @neiljp ! I've only looked into the first three commits, left a couple stylistic comments.

Hmm, because of the fourth commit, it show the three older ones as outdated. Maybe we try first to merge the first three, and then discuss the fourth one?

roberthoenig · 2017-08-15T16:41:06Z

zulip_bots/README.md

@@ -107,9 +107,11 @@ Each bot library simply needs to do the following:

 - Define a class that supports the methods `usage`
 and `handle_message`.
+- Optionally add a class member `ACCEPT_EMPTY_MESSAGES`, to
+specify that empty messages will be passed to the bot.


It would be awesome if you could also add this to bots_guide.md. The guide is quite outdated, but it will be the foundation for a major refactoring.

If these commits go through, rather than the other, then I'll look into that; I wasn't sure if one was outdated, or both should be updated.

roberthoenig · 2017-08-15T16:45:26Z

zulip_bots/zulip_bots/lib.py

@@ -18,6 +18,8 @@

 from zulip import Client

+default_empty_message_response = "Oops. Your message was empty."


Maybe we can plug that string directly in the function, since it only appears one time and your comment down below explains that it is used as a default empty message response.

Sure; I wasn't sure if I'd end up with more strings further down the line.

roberthoenig · 2017-08-15T16:49:38Z

zulip_bots/zulip_bots/lib.py

+    try:
+        handle_empty_here = not (lib_module.handler_class.ACCEPT_EMPTY_MESSAGES)
+    except AttributeError:
+        handle_empty_here = True


handle_empty = getattr(lib_module.handler_class, 'ACCEPT_EMPTY_MESSAGES', True)

Nice catch of a pattern, but if this is changed, I think it probably should be

handle_empty = not getattr(lib_module.handler_class, 'ACCEPT_EMPTY_MESSAGES', False)

That is, if it gets the attr, then True there (bot handles it) implies False to handle_empty (lib handles it); if not present then the result is the same as with yours.

RIght?

Aah, you're right. Complicated boolean logic :) I'd add a comment explaining what's going on here.

Maybe the exception approach is clearer here?

Take inverse of the found variable

...or default to True

I found both confusing, but I prefer gettr just for the sake that it is more conventional.

Fine by me 👍

roberthoenig · 2017-08-15T16:52:17Z

zulip_bots/zulip_bots/lib.py

@@ -197,6 +205,12 @@ def handle_message(message):
            if message['content'] is None:
                return

+        # Handle empty message directly if bot (handler_class) does not
+        # explicitly request to do so
+        if len(message['content']) == 0 and handle_empty_here:


if not message['content'] and handle_empty

roberthoenig · 2017-08-15T16:55:00Z

zulip_bots/zulip_bots/lib.py

+        # explicitly request to do so
+        if len(message['content']) == 0 and handle_empty_here:
+            restricted_client.send_reply(message, default_empty_message_response)
+            return


I'm not sure, but wouldn't the return here cause the whole script to stop?

a) It doesn't seem to ;)
b) This return is from handle_message(), which is called by the lib for each message;
c) A few lines above, it returns, and not for exceptional circumstances, which indicates it should be fine.

All good points, somehow missed that it was in handle_message.

neiljp · 2017-08-15T17:59:04Z

Pushed some changes. Last commit is also in separate PR #66, which might be easier place to discuss.

Thanks for the review @derAnfaenger 👍

Empty messages sent to the bot now trigger a response from within the bot library by default. To handle empty messages explicitly within a bot, a bot class must now define ACCEPT_EMPTY_MESSAGES such that it evaluates to True.

roberthoenig · 2017-08-16T08:39:15Z

@neiljp I like your default commands PR, will give that one a look first, so this one might actually become superfluous :) If we are going to look furter into this PR here, the last commit should be removed.

neiljp · 2017-08-23T01:34:27Z

As per the comment by @derAnfaenger, this is a backup PR which was the origin of the work now in #66, in case #66 is considered too extensive. I'll update this when a decision is made wrt that.

zulipbot · 2017-09-29T23:03:16Z

Heads up @neiljp, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

showell · 2017-11-10T18:22:25Z

zulip_bots/zulip_bots/bots/wikipedia/wikipedia.py

@@ -18,6 +18,8 @@ class WikipediaHandler(object):
    kind of external issue tracker as well.
    '''

+    ACCEPT_EMPTY_MESSAGES = 1


This should be True, correct?

zulipbot · 2017-11-29T18:08:53Z

Heads up @neiljp, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot · 2017-12-09T22:21:52Z

Hello @neiljp, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

neiljp · 2017-12-10T00:32:04Z

Superseded by #225, so closing this previous alternative implementation.

zulipbot added the needs review label Aug 12, 2017

neiljp force-pushed the empty_messages branch from d6a98be to a90d368 Compare August 15, 2017 03:49

zulipbot added reviewed and removed needs review labels Aug 15, 2017

roberthoenig reviewed Aug 15, 2017

View reviewed changes

neiljp force-pushed the empty_messages branch from a90d368 to 49217e1 Compare August 15, 2017 17:50

zulipbot added needs review and removed reviewed labels Aug 15, 2017

neiljp mentioned this pull request Aug 15, 2017

Bots: Add metadata scheme for bots. #66

Open

zulipbot added reviewed and removed needs review labels Aug 15, 2017

neiljp added 4 commits August 15, 2017 11:39

Bots: Handle empty messages in bot lib by default.

617817b

Empty messages sent to the bot now trigger a response from within the bot library by default. To handle empty messages explicitly within a bot, a bot class must now define ACCEPT_EMPTY_MESSAGES such that it evaluates to True.

Docs: Amend README.md to mention ACCEPT_EMPTY_MESSAGES.

b39ad71

Bots: Allow wikipedia bot to accept empty messages.

1885a14

Bots: Alternative extended default commands approach.

20e5bbc

neiljp force-pushed the empty_messages branch from 49217e1 to 20e5bbc Compare August 15, 2017 18:41

zulipbot added needs review and removed reviewed labels Aug 15, 2017

neiljp mentioned this pull request Aug 30, 2017

Handle empty bot messages through the bots framework. #55

Closed

showell force-pushed the master branch from 105db19 to 08e2121 Compare October 21, 2017 00:34

zulipbot added reviewed and removed needs review labels Nov 10, 2017

showell reviewed Nov 10, 2017

View reviewed changes

neiljp mentioned this pull request Dec 9, 2017

Add automatic testing of responses to empty messages #225

Merged

neiljp closed this Dec 10, 2017

zulipbot removed the reviewed label Dec 10, 2017

		@@ -18,6 +18,8 @@

		from zulip import Client

		default_empty_message_response = "Oops. Your message was empty."

Uh oh!

Handle empty messages in bot framework by default #58

Handle empty messages in bot framework by default #58

Uh oh!

Conversation

neiljp commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberthoenig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp commented Aug 15, 2017

Uh oh!

roberthoenig commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp commented Aug 23, 2017

Uh oh!

zulipbot commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zulipbot commented Nov 29, 2017

Uh oh!

zulipbot commented Dec 9, 2017

Uh oh!

neiljp commented Dec 10, 2017

Uh oh!

Uh oh!

neiljp commented Aug 12, 2017 •

edited

Loading

roberthoenig left a comment •

edited

Loading

roberthoenig commented Aug 16, 2017 •

edited

Loading