Merge lp:~isoschiz/endroid/crontaskdecorator into lp:endroid

[Martin Morrison]

Merge in new @task handler.

Adds a @task decorator in the cron module that can be used to decorate methods that are to be used as timed callback handlers. The decorator converts these into a Task object, via the use of a descriptor (which ensures the Task is created by the specific cron associated with the given plugin: for now, there is a single Cron, but this code is future proof for when we have per-plugin Crons).

Also has minor tweaks to help and ratelimit to use some new features.

[ChrisD (gingerchris)]

What's with the lowercase d?
Also the decorator class could do with more comments.
Can you add comments around all functools calls as well?

Finally, I think this might break some usage of persistent crons because it appears to delay registration until the task is used (if I'm following correctly). This might mean after a restart that a plugin might not have re-registered a persistent cron by he time it fires.

Appreciate the other updates :)

review: Needs Fixing

[Martin Morrison (isoschiz)]

Lower case d is an artefact of the evolution, where it was originally a function. In this case, it doesn't really matter as it's all internal, but I'll change it. I can also add more comments.

Good catch on the deferred registration! Fixing that will require moving the support to the metaclass, which means this all gets a lot simpler anyway - probably no longer need the decorator class.

[Phil Connell (pconnell)]

There's an unrelated changeset in here (everything outside cron.py/pluginmanager.py). Is that being merged separately?

- Missing Task in cron.__all__?

- In the docstring for the task decorator, add ">>> CRON_NAME = 'foo'" to make it clear that it's a string?

- It looks a lot like you're missing a functools.wraps for the task decorator. [aside: would be nice if lp generated -p diffs...]
- Possibly a nicer solution to the above would be to move the update_wrapper call inside Task.__init__.

- Should the attributes set on the decorated fn have leading underscores? Since they're internal implementation details, I'd say they should. Would also be nice if they had a common prefix (e.g. "_cron") to make it obvious which to pick out in e.g. a vars() dump.

- name/fn rather than c/f in the dict comprehension? The correspondence with looping over crons in the new endroid_init would then be more obvious.

review: Approve

[lp:~isoschiz/endroid/crontaskdecorator]

51. By Martin Morrison on 2013-08-21

Markups following review

=== modified file 'src/endroid/cron.py'
--- src/endroid/cron.py	2013-08-13 10:04:19 +0000
+++ src/endroid/cron.py	2013-08-21 16:08:24 +0000
@@ -4,25 +4,68 @@
 # Created by Jonathan Millican
 # -----------------------------------------
 
-from endroid.database import Database as Database
+from endroid.database import Database
+
+from twisted.internet import reactor
+from pytz import timezone
 import datetime
-from twisted.internet import reactor
 import cPickle
-from pytz import timezone
 import logging
+import functools
+
+__all__ = (
+    'task',
+    'Cron',
+)
+
+
+def task(name, persistent=True):
+    """
+    Decorator for method of Plugin classes that are to be used as callbacks for
+    cron tasks.
+
+    This decorator ensures the function is registered with the plugin's cron
+    using the given name and persistent arguments. The decorated function can
+    be used like a Task (returned from Cron.register):
+
+        >>> CRON_NAME = "MyCronName"
+        >>> class Foo(Plugin):
+        ...     @task(CRON_NAME)
+        ...     def repeat(self, *args): pass
+        ...     def message_handler(self, msg):
+        ...         self.repeat.setTimeout(10, params)
+
+    That is it provides the setTimeout and doAtTime methods. The decorated
+    function can also be called as normal, should that be needed.
+
+    """
+    def decorator(fn):
+        fn._cron_iscb = True
+        fn._cron_name = name
+        fn._cron_persistent = persistent
+        return fn
+    return decorator
 
 
 class Task(object):
     """
-    Wrapper object providing direct access to a specific "task".  Obtain an
+    Wrapper object providing direct access to a specific "task". Obtain an
     instance using register on the Cron singleton.
 
+    Can also be obtained by wrapping a callback method using the @task
+    decorator. In either case, the Task object is updated to appear as if it is
+    the underlying function, and can be called as if it is.
+
     """
-    __slots__ = ('name', 'cron')
-
-    def __init__(self, name, cron):
+    def __init__(self, name, cron, fn):
         self.name = name
         self.cron = cron
+        self.fn = fn
+        # Disguise the Task as the function it is wrapping
+        functools.update_wrapper(self, fn)
+
+    def __call__(self, *args, **kwargs):
+        return self.fn(*args, **kwargs)
 
     def doAtTime(self, time, locality, params):
         return self.cron.doAtTime(time, locality, self.name, params)
@@ -94,7 +137,7 @@
             self.removeTask(reg_name)
 
         self.fun_dict.update({reg_name: function})
-        return Task(reg_name, self)
+        return Task(reg_name, self, function)
 
     def cancel(self):
         if self.delayedcall:

=== modified file 'src/endroid/pluginmanager.py'
--- src/endroid/pluginmanager.py	2013-08-14 18:18:15 +0000
+++ src/endroid/pluginmanager.py	2013-08-21 16:08:24 +0000
@@ -6,6 +6,8 @@
 
 import sys
 import logging
+import functools
+
 from endroid.cron import Cron
 from endroid.database import Database
 
@@ -56,6 +58,26 @@
         if 'enInit' in dict:
             dict['endroid_init'] = dict['enInit']
             del dict['enInit']
+
+        # Support for the Cron @task decorator
+        crons = {name: fn for name, fn in dict.items()
+                 if getattr(fn, "_cron_iscb", False)}
+        init = dict.get('endroid_init', lambda _: None)
+        # Make sure the new init function looks like the old one
+        @functools.wraps(init)
+        def endroid_init(self):
+            for name, fn in crons.items():
+                @functools.wraps(fn)
+                def inner(*args, **kwargs):
+                    # This function is here to ensure the right obj is passed
+                    # as self to the method.
+                    return fn(self, *args, **kwargs)
+                task = self.cron.register(inner, fn._cron_name,
+                                          persistent=fn._cron_persistent)
+                setattr(self, name, task)
+            init(self)
+        dict['endroid_init'] = endroid_init
+
         return type.__new__(meta, name, bases, dict)
 
     def __init__(cls, name, bases, dict):

=== modified file 'src/endroid/plugins/command.py'
--- src/endroid/plugins/command.py	2013-08-13 18:38:51 +0000
+++ src/endroid/plugins/command.py	2013-08-21 16:08:24 +0000
@@ -4,6 +4,7 @@
 # Created by Jonathan Millican
 # -----------------------------------------
 
+import functools
 from collections import namedtuple
 from endroid.pluginmanager import Plugin, PluginMeta
 
@@ -72,6 +73,8 @@
         cmds = dict((c, f) for c, f in dct.items()
                     if c.startswith("cmd_") or getattr(f, "is_command", False))
         init = dct.get('endroid_init', lambda _: None)
+        # Make sure the new init function looks like the old one
+        @functools.wraps(init)
         def endroid_init(self):
             com = self.get('endroid.plugins.command')
             for cmd, fn in cmds.items():
@@ -82,6 +85,9 @@
                 else:
                     reg_fn = com.register_both
                 words = cmd.split("_")
+                # Handle a leading underscore (may be necessary in some cases)
+                if not words[0]:
+                    words = words[1:]
                 if not getattr(fn, "is_command", False):
                     words = words[1:]
                 reg_fn(getattr(self, cmd), words,

=== modified file 'src/endroid/plugins/help.py'
--- src/endroid/plugins/help.py	2013-08-11 19:50:24 +0000
+++ src/endroid/plugins/help.py	2013-08-21 16:08:24 +0000
@@ -3,9 +3,11 @@
 # Copyright 2012, Ensoft Ltd
 # -----------------------------------------------------------------------------
 
-from endroid.plugins.command import CommandPlugin
+from endroid.plugins.command import CommandPlugin, command
 
 class Help(CommandPlugin):
+    name = "help"
+
     def endroid_init(self):
         self.help_topics = {
             '': self.show_help_main,
@@ -19,12 +21,13 @@
 
     def load_plugin_list(self):
         self._plugins = {}
-        for fullname in self.list_plugins():
-            plugin = self.get(fullname)
+        for fullname in self.plugins.all():
+            plugin = self.plugins.get(fullname)
             name = getattr(plugin, "name", fullname)
             self._plugins[name] = (fullname, plugin)
 
-    def cmd_help(self, msg, args):
+    @command
+    def _help(self, msg, args):
         msg.reply_to_sender(self.show_help_plugin("help", args))
 
     def show_help_main(self, topic):
@@ -54,7 +57,7 @@
     def show_help_plugin(self, name, topic=''):
         out = []
         fullname, plugin = self._plugins.get(name, (name, None))
-        if self.pluginLoaded(fullname):
+        if self.plugins.loaded(fullname):
             # First check if it has a simple "help" property or method
             if hasattr(plugin, "help"):
                 try:

=== modified file 'src/endroid/plugins/ratelimit.py'
--- src/endroid/plugins/ratelimit.py	2013-08-14 18:18:15 +0000
+++ src/endroid/plugins/ratelimit.py	2013-08-21 16:08:24 +0000
@@ -11,6 +11,7 @@
 
 from endroid.pluginmanager import Plugin
 from endroid.messagehandler import Priority
+from endroid.cron import task
 
 # Cron constants
 CRON_SENDAGAIN = "RateLimit_SendAgain"
@@ -178,8 +179,6 @@
 
         RateLimit.waitingusers = set()
 
-        self.task = self.cron.register(self.sendagain, CRON_SENDAGAIN)
-
     def ratelimit(self, msg):
         """
         Send message filter. Rate limits based on the message recipient, using
@@ -189,7 +188,7 @@
         messages. TODO: store queued messages in the DB so they survive a
         process restart?
 
-        PRIORITY_URGENT messages are not rate limited.
+        Priority.URGENT messages are not rate limited.
         """
         sc = self.limiters[msg.recipient]
 
@@ -229,6 +228,7 @@
 
         return True
 
+    @task(CRON_SENDAGAIN)
     def sendagain(self, user):
         """
         Cron callback handler. Attempts to send as many messages as it can from
@@ -265,4 +265,4 @@
         if len(sc.queue) > 0 and user not in self.waitingusers:
             self.waitingusers.add(user)
             timedelta = 1.0 / self.fillrate
-            self.task.setTimeout(timedelta, user)
+            self.sendagain.setTimeout(timedelta, user)

=== modified file 'src/endroid/usermanagement.py'
--- src/endroid/usermanagement.py	2013-08-19 15:06:08 +0000
+++ src/endroid/usermanagement.py	2013-08-21 16:08:24 +0000
@@ -603,19 +603,18 @@
         """
         try:
             return JID(item).userhost()
-            # will throw a RuntimeError if item is None
+            # will raise a RuntimeError if item is None
             # or an InvalidFormat if item is a string but not formatted properly
-            # If the item is not even a string, will raise attributeerror
+            # If the item is not even a string, will raise AttributeError
         except (RuntimeError, InvalidFormat, AttributeError):
             return item
-            # if item is not a string or None, an AttributeError will be raised
 
     def for_plugin(self, pluginmanager, plugin):
         return PluginUserManagement(self, pluginmanager, plugin)
 
 class PluginUserManagement(object):
     """
-    One of these exists per plugin, provide the API to handle rosters.
+    One of these exists per plugin, provides the API to handle rosters.
     """
     def __init__(self, usermanagement, pluginmanager, plugin):
         self._usermanagement = usermanagement