Merge lp:~stefanor/ibid/calc-525439 into lp:~ibid-core/ibid/old-trunk-1.6

[Stefano Rivera]

[Michael Gorven (mgorven)]

 review approve

review: Approve

[Michael Gorven (mgorven)]

Hackity hack...
 review approve

review: Approve

[Stefano Rivera (stefanor)]

21:10 <@cocooncrash> tumbleweed: Er, I don't quite understand what you're doing in calc
21:11 < tumbleweed> ok, let me explain what I had to change

here goes:

Previously module authors could bypass the Processor inspection (to locate handler functions) by providing a class level list "_event_handlers" of handlers, otherwise it inspects and builds a list of the same name.

Problem with that is when you inherit from a Processor then the subclass assumes that the existing _event_handlers was manual, when it really comes from inspecting the base class.

Solution:
Store the inspected list in __event_handlers (which has to be manually mangled because setattr et al. don't mangle). And extend the list during subclass initialisation. Overriding with a manual list works as before.

[Michael Gorven (mgorven)]

review: Approve

[Keegan Carruthers-Smith (keegan-csmith)]

Is _event_handlers or _periodic_handlers ever not None now?

[Stefano Rivera (stefanor)]

> Is _event_handlers or _periodic_handlers ever not None now?

Nope. And _get_*_handlers isn't overridden by anyone either.

[Keegan Carruthers-Smith (keegan-csmith)]

Ok, I didn't see the comment explaining the code change when I commented. So a plugin can choose to manually override that.

I haven't really ever played with __new__ before, so I am not 100% what goes on with the setattr(). Say a subclass, A, of processor has _Processor__event_handlers as [handler_a]. Then B, a subclass of A, adds handler_b. So B._Processor__event_handlers is [handler_a, handler_b]? If so I am led to believe A. and Processor. _Processor__event_handlers is also [handler_a, handler_b]. Just wondering if this is intended and if this is how it works?

Also, maybe _Processor__*_handlers should be sets instead of lists?

review: Needs Information

[Stefano Rivera (stefanor)]

> I haven't really ever played with __new__ before, so I am not 100% what goes
> on with the setattr().

setattr(a, 'b', 'c') is the same as a.b = c
however, setattr doesn't know to do name mangling if you say setattr(a, '__b')

> Say a subclass, A, of processor has _Processor__event_handlers as [handler_a].
> Then B, a subclass of A, adds handler_b. So B._Processor__event_handlers is
> [handler_a, handler_b]?

Correct

> If so I am led to believe A. and Processor. _Processor__event_handlers is also
> [handler_a, handler_b]. Just wondering if this is intended and if this is how
> it works?

Ok, that bit I don't understand. Explain to me on IRC

> Also, maybe _Processor__*_handlers should be sets instead of lists?

No. Definable order could be handy.

[Keegan Carruthers-Smith (keegan-csmith)]

review: Approve

[Jonathan Hitchcock (vhata)]

review: Approve

=== modified file 'ibid/plugins/__init__.py'
--- ibid/plugins/__init__.py	2010-02-03 15:04:43 +0000
+++ ibid/plugins/__init__.py	2010-02-23 11:48:19 +0000
@@ -62,16 +62,17 @@
             new.default = getattr(cls, name)
             setattr(cls, name, new)
 
-        if getattr(cls, '_event_handlers', None) is None:
-            cls._event_handlers = []
-            for name, item in cls.__dict__.iteritems():
-                if getattr(item, 'handler', False):
-                    cls._event_handlers.append(name)
-        if getattr(cls, '_periodic_handlers', None) is None:
-            cls._periodic_handlers = []
-            for name, item in cls.__dict__.iteritems():
-                if getattr(item, 'periodic', False):
-                    cls._periodic_handlers.append(name)
+        for attr, listname in (
+                ('handler', 'event'),
+                ('periodic', 'periodic')):
+            listname = '_Processor__%s_handlers' % listname
+            if not hasattr(cls, listname):
+                setattr(cls, listname, [])
+            handlers = getattr(cls, listname)
+
+            for name, item in cls.__dict__.iteritems():
+                if getattr(item, attr, False) and name not in handlers:
+                    handlers.append(name)
 
         return super(Processor, cls).__new__(cls)
 
@@ -127,12 +128,12 @@
 
     def _get_event_handlers(self):
         "Find all the handlers (regex matching and blind)"
-        for handler in self._event_handlers:
+        for handler in self._event_handlers or self.__event_handlers:
             yield getattr(self, handler)
 
     def _get_periodic_handlers(self):
         "Find all the periodic handlers"
-        for handler in self._periodic_handlers:
+        for handler in self._periodic_handlers or self.__periodic_handlers:
             yield getattr(self, handler)
 
     def _run_periodic_handler(self, method, event):

=== modified file 'ibid/plugins/calc.py'
--- ibid/plugins/calc.py	2010-01-18 23:20:33 +0000
+++ ibid/plugins/calc.py	2010-02-23 11:48:19 +0000
@@ -2,10 +2,9 @@
 # Released under terms of the MIT/X/Expat Licence. See COPYING for details.
 
 from __future__ import division
-from random import random, randint
-
 import logging
 from os import kill
+from random import random, randint
 from signal import SIGTERM
 from subprocess import Popen, PIPE
 from time import time, sleep
@@ -151,7 +150,7 @@
     safe['pow'] = limited_pow
     safe['factorial'] = limited_factorial
 
-    @match(r'^(?:calc\s+)?(.+?)$')
+    @match(r'^(.+)$')
     def calculate(self, event, expression):
         for term in self.banned:
             if term in expression:
@@ -193,6 +192,14 @@
         if isinstance(result, (int, long, float, complex)):
             event.addresponse(unicode(result))
 
+
+class ExplicitCalc(Calc):
+    priority = 0
+
+    @match(r'^calc(?:ulate)?\s+(.+)$')
+    def calculate(self, event, expression):
+        super(ExplicitCalc, self).calculate(event, expression)
+
 help['random'] = u'Generates random numbers.'
 class Random(Processor):
     u"""random [ <max> | <min> <max> ]"""