Refactor command replies, again

Make EventPlugin.respond_to always return a Deferred.

Break apart abstract connection and command test cases into mixins
instead of TestCase subclasses.

Don't invariably return a Deferred from ReplyBuffer.next.

Provide a more appropriate default message if no results are returned.
Let commands return an empty sequence or iterator instead of raising a
message on their own when there are no results.

docs/api.rst

@@ -38,7 +38,6 @@ Connections
 .. autoclass:: ChannelUserInfo
    :members:
 
-.. autodata:: CHUNK_LENGTH
 .. autodata:: MAX_REPLY_LENGTH
 
 
@@ -68,6 +67,8 @@ Messages
    To create a new object based on the attributes of an existing one,
    use an instance's `~collections.somenamedtuple._replace` method.
 
+.. autodata:: CHUNK_LENGTH
+
 
 .. _message-types:
 

docs/plugins.rst

@@ -92,6 +92,11 @@ either a channel message addressed to the command target or a private
 notice depending on how the command was invoked.
 A command reply may take one of the following forms:
 
+* `None`, in which case no reply is shown to the target user, not even a
+  "no results" message.
+  This is useful for commands that have other visible side effects, such
+  as changing the channel topic or mode.
+
 * A byte or Unicode string.
   Long strings are broken into chunks of up to `.CHUNK_LENGTH` bytes and
   treated as a sequence.
@@ -106,9 +111,9 @@ A command reply may take one of the following forms:
 
 * A `~twisted.internet.defer.Deferred` object yielding any of the above.
 
-In all cases, the first reply is immediately shown to the target user,
-and any remaining replies are placed in a buffer for later retrieval
-using the `.more <.plugins.more>` command.
+Unless the reply is `None`, the first reply is immediately shown to the
+target user, and any remaining replies are placed in a buffer for later
+retrieval using the `.more <.plugins.more>` command.
 Newlines inside replies are displayed as a slash surrounded by spaces.
 
 The following example plugin implements an infinite counter::

omnipresence/connection.py

@@ -417,9 +417,7 @@ def respond_to(self, msg):
                 # every plugin active on this connection.
                 plugins.update(self.settings.loaded_plugins.itervalues())
             for plugin in plugins:
-                self.log.debug('Passing message {msg} to plugin {name}',
-                               name=plugin.__class__.name, msg=msg)
-                deferred = maybeDeferred(plugin.respond_to, msg)
+                deferred = plugin.respond_to(msg)
                 if msg.action == 'command':
                     deferred.addCallback(self.buffer_and_reply, msg)
                     deferred.addErrback(self.reply_from_error, msg)
@@ -456,11 +454,11 @@ def buffer_and_reply(self, response, request):
         the appropriate user's reply buffer according to the invocation
         `.Message` *request*, and reply with the first message."""
         venue = PRIVATE_CHANNEL if request.private else request.venue
-        if not response:
+        if response is None:
             self.message_buffers[venue].pop(request.actor.nick, None)
             returnValue(None)
         buf = ReplyBuffer(response, request)
-        reply_string = (yield next(buf)) or 'No text in buffer.'
+        reply_string = (yield maybeDeferred(next, buf, None)) or 'No results.'
         remaining = length_hint(buf)
         tail = ' (+{} more)'.format(remaining) if remaining else ''
         self.message_buffers[venue][request.actor.nick] = buf

omnipresence/message/__init__.py

@@ -6,8 +6,6 @@
 from functools import partial
 import itertools
 
-from twisted.internet.defer import maybeDeferred, succeed
-
 from ..compat import length_hint
 from ..hostmask import Hostmask
 from .formatting import remove_formatting, unclosed_formatting
@@ -254,12 +252,13 @@ def __init__(self, response, request=None):
                             type(response).__name__)
 
     def next(self):
-        """Return a `Deferred` yielding the next reply string."""
         if isinstance(self.response, Sequence):
-            reply_string = self.response[0] if self.response else None
+            if not self.response:
+                raise StopIteration
+            reply_string = self.response[0]
             self.response = self.response[1:]
-            return succeed(reply_string)
-        return maybeDeferred(next, self.response, None)
+            return reply_string
+        return next(self.response)
 
     def tee(self, num=2):
         """Return *num* independent reply buffers from this one, like

omnipresence/plugin.py

@@ -21,6 +21,9 @@
 
 import importlib
 
+from twisted.internet.defer import maybeDeferred, succeed
+from twisted.logger import Logger
+
 
 #: The root package name to use for relative plugin module searches.
 PLUGIN_ROOT = 'omnipresence.plugins'
@@ -43,16 +46,21 @@ class EventPlugin(object):
 
     __metaclass__ = EventPluginMeta
 
+    log = Logger()
+
     def respond_to(self, msg):
-        """Start any callback this plugin defines for *msg*, and forward
-        its return value, or `None` if no such callback exists."""
+        """Start any callback this plugin defines for *msg*.  Return a
+        `Deferred` yielding its return value, or `None` if no callback
+        exists for this message."""
         callback_name = 'on_' + msg.action
         if not hasattr(self, callback_name):
-            return
+            return succeed(None)
         callback = getattr(self, callback_name)
         if msg.outgoing and not getattr(callback, 'outgoing', False):
-            return
-        return callback(msg)
+            return succeed(None)
+        self.log.debug('Passing message {msg} to {plugin} callback {name}',
+                       msg=msg, plugin=type(self).name, name=callback_name)
+        return maybeDeferred(callback, msg)
 
 
 def load_plugin(name):

omnipresence/plugins/anidb/__init__.py

@@ -106,17 +106,13 @@ def on_command(self, msg):
         content = yield readBody(response)
         soup = BeautifulSoup(content)
 
-        # We may get one of three differently-formatted responses
-        # depending on the number of results: if there are no results,
-        # an animelist page with a notice; if there are multiple
-        # results, an animelist page with a table containing them; and
-        # if there is only one result, an individual anime page.
+        # We get one of two response formats, depending on the number
+        # of results.  If there is exactly one result, we're redirected
+        # to the individual anime page, which has an "anime_all" table.
+        # Otherwise, we get an anime listing.
         results = parse_animelist(soup.find('table', 'animelist'))
         if not results:
             results = parse_anime_all(soup.find('div', 'anime_all'))
-        if not results:
-            raise UserVisibleError('No results found for \x02{}\x02.'
-                                   .format(msg.content))
         messages = []
         for anime in results:
             anime = {k: v.strip() for k, v in anime.iteritems()}

omnipresence/plugins/anidb/test_anidb.py

@@ -3,47 +3,57 @@
 # pylint: disable=missing-docstring,too-few-public-methods
 
 
+from twisted.internet.defer import inlineCallbacks
+from twisted.trial.unittest import TestCase
+
 from ...message import collapse
-from ...test.helpers import AbstractCassetteTestCase
+from ...test.helpers import CommandTestMixin
 
 from . import Default
 
 
-class AniDBTestCase(AbstractCassetteTestCase):
+class AniDBTestCase(CommandTestMixin, TestCase):
     command_class = Default
 
-    @AbstractCassetteTestCase.use_cassette('anidb/no-results')
+    @CommandTestMixin.use_cassette('anidb/no-results')
+    @inlineCallbacks
     def test_no_results(self):
-        return self.assert_error(
-            'slartibartfast',
-            'No results found for \x02slartibartfast\x02.')
+        yield self.send_command('slartibartfast')
+        yield self.assert_no_replies()
 
-    @AbstractCassetteTestCase.use_cassette('anidb/single-result')
+    @CommandTestMixin.use_cassette('anidb/single-result')
+    @inlineCallbacks
     def test_single_result(self):
-        return self.assert_reply(
-            'bakemonogatari',
-            [collapse(u"""\
-                http://anidb.net/a6327 —
-                \x02Bakemonogatari\x02 —
-                TV Series, 12 episodes from 2009-07-03 to 2009-09-25 —
-                rated 8.43 (7443)""")])
-
-    @AbstractCassetteTestCase.use_cassette('anidb/multiple-results')
+        yield self.send_command('bakemonogatari')
+        yield self.assert_reply(collapse(u"""\
+            http://anidb.net/a6327 —
+            \x02Bakemonogatari\x02 —
+            TV Series, 12 episodes from 2009-07-03 to 2009-09-25 —
+            rated 8.43 (7443)"""))
+        yield self.assert_no_replies()
+
+    @CommandTestMixin.use_cassette('anidb/multiple-results')
+    @inlineCallbacks
     def test_multiple_results(self):
-        return self.assert_reply('suzumiya', map(collapse, [
-            u"""http://anidb.net/a3651 —
-                \x02Suzumiya Haruhi no Yuuutsu\x02 —
-                TV Series, 14 episodes from 2006-04-03 to 2006-07-03 —
-                rated 8.16 (13583)""",
-            u"""http://anidb.net/a6367 —
-                \x02Suzumiya Haruhi no Yuuutsu (2009)\x02 —
-                TV Series, 28 episodes from 2009-04-03 to 2009-10-09 —
-                rated 5.18 (4482)""",
-            u"""http://anidb.net/a7221 —
-                \x02Suzumiya Haruhi no Shoushitsu\x02 —
-                Movie, 1 episode on 2010-02-06 —
-                rated 9.41 (4208)""",
-            u"""http://anidb.net/a5335 —
-                \x02Suzumiya Haruhi-chan no Yuuutsu\x02 —
-                Web, 25 episodes from 2009-02-14 to 2009-05-08 —
-                rated 5.32 (1635)"""]))
+        yield self.send_command('suzumiya')
+        yield self.assert_reply(collapse(u"""\
+            http://anidb.net/a3651 —
+            \x02Suzumiya Haruhi no Yuuutsu\x02 —
+            TV Series, 14 episodes from 2006-04-03 to 2006-07-03 —
+            rated 8.16 (13583)"""))
+        yield self.assert_reply(collapse(u"""\
+            http://anidb.net/a6367 —
+            \x02Suzumiya Haruhi no Yuuutsu (2009)\x02 —
+            TV Series, 28 episodes from 2009-04-03 to 2009-10-09 —
+            rated 5.18 (4482)"""))
+        yield self.assert_reply(collapse(u"""\
+            http://anidb.net/a7221 —
+            \x02Suzumiya Haruhi no Shoushitsu\x02 —
+            Movie, 1 episode on 2010-02-06 —
+            rated 9.41 (4208)"""))
+        yield self.assert_reply(collapse(u"""\
+            http://anidb.net/a5335 —
+            \x02Suzumiya Haruhi-chan no Yuuutsu\x02 —
+            Web, 25 episodes from 2009-02-14 to 2009-05-08 —
+            rated 5.32 (1635)"""))
+        yield self.assert_no_replies()

omnipresence/plugins/dice/test_dice.py

@@ -2,9 +2,12 @@
 # pylint: disable=missing-docstring,too-few-public-methods
 
 
+from twisted.internet.defer import inlineCallbacks
+from twisted.trial.unittest import TestCase
+
 from ...hostmask import Hostmask
 from ...message import Message, collapse
-from ...test.helpers import AbstractCommandTestCase
+from ...test.helpers import CommandTestMixin
 
 from . import Default
 
@@ -14,75 +17,98 @@ def randint(self, a, b):
         return b
 
 
-class DiceTestCase(AbstractCommandTestCase):
+class DiceTestCase(CommandTestMixin, TestCase):
     command_class = Default
 
     def setUp(self):
         super(DiceTestCase, self).setUp()
         self.command.random = DummyRandom()
 
+    @inlineCallbacks
     def test_show(self):
-        self.assert_reply('', 'Bank has no rolls.')
-        self.assert_reply('show', 'Bank has no rolls.')
+        yield self.send_command('')
+        yield self.assert_reply('Bank has no rolls.')
+        yield self.send_command('show')
+        yield self.assert_reply('Bank has no rolls.')
 
+    @inlineCallbacks
     def test_simple_rolls(self):
-        self.assert_reply('roll 4 d6 3d10 4d8',
-                          'Rolled \x024 6 8 8 8 8 10 10 10\x02 = 72.')
+        yield self.send_command('roll 4 d6 3d10 4d8')
+        yield self.assert_reply('Rolled \x024 6 8 8 8 8 10 10 10\x02 = 72.')
 
+    @inlineCallbacks
     def test_bad_rolls(self):
-        self.assert_error('roll asdf',
-                          'Invalid die group specification asdf.')
-        self.assert_error('roll -1d16', 'Invalid number of dice -1.')
-        self.assert_error('roll 1d-20', 'Invalid die size -20.')
+        yield self.send_command('roll 2d6 asdf')
+        self.assert_error('Invalid die group specification asdf.')
+        yield self.send_command('roll -1d16')
+        self.assert_error('Invalid number of dice -1.')
+        yield self.send_command('roll 1d-20')
+        self.assert_error('Invalid die size -20.')
 
+    @inlineCallbacks
     def test_bank_accumulation(self):
-        self.assert_reply('add d6 3d10 4d8', collapse("""\
+        yield self.send_command('add d6 3d10 4d8')
+        yield self.assert_reply(collapse("""\
             Rolled \x026 8 8 8 8 10 10 10\x02 = 68.
             Bank now has \x026 8 8 8 8 10 10 10\x02 = 68.
             """))
-        self.assert_reply('add d4', collapse("""\
+        yield self.send_command('add d4')
+        yield self.assert_reply(collapse("""\
             Rolled \x024\x02 = 4.
             Bank now has \x024 6 8 8 8 8 10 10 10\x02 = 72.
             """))
-        self.assert_reply('new d4', collapse("""\
+        yield self.send_command('new d4')
+        yield self.assert_reply(collapse("""\
             Rolled \x024\x02 = 4.
             Bank now has \x024\x02 = 4.
             """))
-        self.assert_reply('clear', 'Bank cleared.')
-        self.assert_reply('', 'Bank has no rolls.')
+        yield self.send_command('clear')
+        yield self.assert_reply('Bank cleared.')
+        yield self.send_command('')
+        yield self.assert_reply('Bank has no rolls.')
 
+    @inlineCallbacks
     def test_bank_isolation(self):
-        self.assert_reply('add d6 3d10 4d8', collapse("""\
+        yield self.send_command('add d6 3d10 4d8')
+        yield self.assert_reply(collapse("""\
             Rolled \x026 8 8 8 8 10 10 10\x02 = 68.
             Bank now has \x026 8 8 8 8 10 10 10\x02 = 68.
             """))
-        self.assert_reply('', 'Bank has no rolls.',
-                          actor=Hostmask('party3', 'user', 'host'))
-        self.assert_reply('show {}'.format(self.other_user.nick),
-                          'Bank has \x026 8 8 8 8 10 10 10\x02 = 68.')
+        yield self.send_command('', actor=self.other_users[1])
+        yield self.assert_reply('Bank has no rolls.')
+        yield self.send_command('show {}'.format(self.other_users[0].nick),
+                                actor=self.other_users[1])
+        yield self.assert_reply('Bank has \x026 8 8 8 8 10 10 10\x02 = 68.')
 
+    @inlineCallbacks
     def test_bank_follows_nick(self):
-        self.assert_reply('add d6 3d10 4d8', collapse("""\
+        yield self.send_command('add d6 3d10 4d8')
+        yield self.assert_reply(collapse("""\
             Rolled \x026 8 8 8 8 10 10 10\x02 = 68.
             Bank now has \x026 8 8 8 8 10 10 10\x02 = 68.
             """))
-        self.command.respond_to(Message(
+        yield self.command.respond_to(Message(
             self.connection, False, 'nick',
-            actor=self.other_user, content='party3'))
-        self.assert_reply('', 'Bank has no rolls.')
-        self.assert_reply('', 'Bank has \x026 8 8 8 8 10 10 10\x02 = 68.',
-                          actor=Hostmask('party3', 'user', 'host'))
+            actor=self.other_users[0], content=self.other_users[1].nick))
+        yield self.send_command('')
+        yield self.assert_reply('Bank has no rolls.')
+        yield self.send_command('', actor=self.other_users[1])
+        yield self.assert_reply('Bank has \x026 8 8 8 8 10 10 10\x02 = 68.')
 
+    @inlineCallbacks
     def test_use(self):
-        self.assert_reply('add d6 3d10 4d8', collapse("""\
+        yield self.send_command('add d6 3d10 4d8')
+        yield self.assert_reply(collapse("""\
             Rolled \x026 8 8 8 8 10 10 10\x02 = 68.
             Bank now has \x026 8 8 8 8 10 10 10\x02 = 68.
             """))
-        self.assert_reply('use 6 8 10', collapse("""\
+        yield self.send_command('use 6 8 10')
+        yield self.assert_reply(collapse("""\
             Used \x026 8 10\x02 = 24.
             Bank now has \x028 8 8 10 10\x02 = 44.
             """))
-        self.assert_error('use 1 2 3 8', collapse("""\
+        yield self.send_command('use 1 2 3 8')
+        self.assert_error(collapse("""\
             You do not have enough 1s, 2s, and 3s in your die bank to
             use those rolls.
             """))

omnipresence/plugins/google/__init__.py

@@ -61,8 +61,8 @@ def next(self):
                                    data['error']['message'])
         self.items = data.get('items')
         if not self.items:
-            raise UserVisibleError('No results found for \x02{}\x02.'
-                                   .format(self.q))
+            self.start = None
+            returnValue(None)
         if self.start == 1:
             self.total_results = int(data['searchInformation']['totalResults'])
         if 'nextPage' in data['queries']: