launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/python-oops/extraction into lp:python-oops.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lifeless/python-oops/extraction/+merge/71644

Fixes the template to be safe with lists / dicts in it; provide a filtering API for the filtering we do in LP; permit a context to be passed through create() to the on_create hooks, which permits splitting up the monolithic _makeReport in Launchpad.
-- 
https://code.launchpad.net/~lifeless/python-oops/extraction/+merge/71644
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops/extraction into lp:python-oops.

=== modified file 'README'
--- README	2011-08-15 08:41:43 +0000
+++ README	2011-08-16 07:06:22 +0000
@@ -54,7 +54,7 @@
   >>> from oops import Config
   >>> config = Config()
 
-* New report will be based on the template::
+* New reports will be based on the template report::
 
   >>> config.template
   {}
@@ -68,13 +68,18 @@
 when the oops is created, or to override / tweak the information gathered by an
 earlier callback)::
 
-  >>> mycallback = lambda report: None
+  >>> mycallback = lambda report, context: None
   >>> config.on_create.append(mycallback)
 
+The context parameter is also just dict, and is passed to all the on_create
+callbacks similarly to the report. This is used to support passing information
+to the on_create hooks. For instance, the exc_info key is used to pass in
+information about the exception being logged (if one was caught).
+
 * Later on, when something has gone wrong and you want to create an OOPS
 report::
 
-  >>> report = config.create()
+  >>> report = config.create(context=dict(exc_info=sys.exc_info()))
   >>> report
   {'appname': 'demo', 'branch_nick': 'mybranch'}
 
@@ -88,6 +93,9 @@
 to the repository). Publishers should try to use report['id'] for the id, if it
 is set. This is automatically set to the id returned by the previous publisher.
 
+If publish returns None, then the report was filtered (see the api docs for
+more information).
+
   >>> 'id' in report
   False
   >>> def demo_publish(report):
@@ -98,7 +106,7 @@
   >>> report['id']
   'id 1'
 
-* The oops_datedir_repo package contains a local disk based repository which
+* The related project oops_datedir_repo contains a local disk based repository which
 can be used as a publisher.
 
 More coming soon.

=== modified file 'oops/__init__.py'
--- oops/__init__.py	2011-08-15 06:42:14 +0000
+++ oops/__init__.py	2011-08-16 07:06:22 +0000
@@ -25,7 +25,7 @@
 # established at this point, and setup.py will use a version of next-$(revno).
 # If the releaselevel is 'final', then the tarball will be major.minor.micro.
 # Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 4, 'beta', 0)
+__version__ = (0, 0, 5, 'beta', 0)
 
 __all__ = [
     'Config'

=== modified file 'oops/config.py'
--- oops/config.py	2011-08-15 08:41:43 +0000
+++ oops/config.py	2011-08-16 07:06:22 +0000
@@ -70,13 +70,20 @@
 
 __metaclass__ = type
 
+from copy import deepcopy
+
 
 class Config:
     """The configuration for the OOPS system.
     
     :ivar on_create: A list of callables to call when making a new report. Each
-        will be called in series with the new report, and its return value will
-        be ignored.
+        will be called in series with the new report and a creation context
+        dict. The return value of the callbacks is ignored.
+    :ivar filters: A list of callables to call when filtering a report. Each
+        will be called in series with a report that is about to be published.
+        If the filter returns true (that is not None, 0, '' or False), then 
+        the report will not be published, and the call to publish will return
+        None to the user.
     :ivar publishers: A list of callables to call when publishing a report.
         Each will be called in series with the report to publish. Their return
         value will be assigned to the reports 'id' key : if a publisher
@@ -86,22 +93,30 @@
     """
 
     def __init__(self):
+        self.filters = []
         self.on_create = []
         self.template = {}
         self.publishers = []
 
-    def create(self):
+    def create(self, context=None):
         """Create an OOPS.
 
-        The current template used copied to make the new report, and it is
-        passed to all the on_create callbacks for population.
+        The current template is copied to make the new report, and the new
+        report is then passed to all the on_create callbacks for population.
 
         If a callback raises an exception, that will propgate to the caller.
 
+        :param context: A dict of information that the on_create callbacks can
+            use in populating the report. For instance, the attach_exception 
+            callback looks for an exc_info key in the context and uses that
+            to add information to the report. If context is None, an empty dict
+            is created and used with the callbacks.
         :return: A fresh OOPS.
         """
-        result = dict(self.template)
-        [callback(result) for callback in self.on_create]
+        if context is None:
+            context = {}
+        result = deepcopy(self.template)
+        [callback(result, context) for callback in self.on_create]
         return result
 
     def publish(self, report):
@@ -119,6 +134,9 @@
 
         :return: A list of the allocated ids.
         """
+        for report_filter in self.filters:
+            if report_filter(report):
+                return None
         result = []
         for publisher in self.publishers:
             id = publisher(report)

=== modified file 'oops/tests/test_config.py'
--- oops/tests/test_config.py	2011-08-15 08:41:43 +0000
+++ oops/tests/test_config.py	2011-08-16 07:06:22 +0000
@@ -36,21 +36,32 @@
 
     def test_on_create_called(self):
         calls = []
-        def capture(id, report):
-            calls.append((id, report))
+        def capture(id, report, context):
+            calls.append((id, report, context))
         config = Config()
         config.on_create.append(partial(capture, '1'))
         config.on_create.append(partial(capture, '2'))
-        report = config.create()
+        report = config.create('2')
         self.assertThat(report, Is(calls[0][1]))
         self.assertThat(report, Is(calls[1][1]))
-        self.assertEqual([('1', {}), ('2', {})], calls)
+        self.assertEqual([('1', {}, '2'), ('2', {}, '2')], calls)
+
+    def test_on_create_no_context(self):
+        calls = []
+        def capture(report, context):
+            calls.append((report, context))
+        config = Config()
+        config.on_create.append(capture)
+        report = config.create()
+        self.assertEqual([({}, {})], calls)
 
     def test_create_template(self):
         config = Config()
         config.template['base'] = True
+        config.template['list'] = []
         report = config.create()
-        self.assertEqual({'base': True}, report)
+        self.assertEqual({'base': True, 'list': []}, report)
+        self.assertIsNot(report['list'], config.template['list'])
 
     def test_publish_calls_publishers(self):
         calls = []
@@ -66,3 +77,21 @@
         self.assertEqual(['1', '2'], config.publish(report))
         self.assertEqual({'id': '2'}, report)
 
+    def test_publish_filters_first(self):
+        calls = []
+        def filter_ok(report):
+            calls.append('ok')
+        def filter_block(report):
+            calls.append('block')
+            return True
+        def pub(report):
+            calls.append('pub')
+            return '1'
+        config = Config()
+        config.filters.append(filter_ok)
+        config.filters.append(filter_block)
+        config.filters.append(filter_ok)
+        config.publishers.append(pub)
+        report = {}
+        self.assertEqual(None, config.publish(report))
+        self.assertEqual(['ok', 'block'], calls)

=== modified file 'setup.py'
--- setup.py	2011-08-15 08:41:43 +0000
+++ setup.py	2011-08-16 07:06:22 +0000
@@ -23,7 +23,7 @@
         os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
 
 setup(name="oops",
-      version="0.0.4",
+      version="0.0.5",
       description=\
               "OOPS report model and default allocation/[de]serialization.",
       long_description=description,