launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

0.0.9                         
-----
                               
* Do not propogate oops ids of None to the report: they indicate that the oops was not handled. (Robert Collins)
                            
* New helper function ``publish_new_only`` which lets you easily add fallback publishers: if the first publisher doesn't and returns None, ``publish_new_only`` will call the publisher it wraps. Otherwise it will just return None itself. (Robert Collins)
-- 
https://code.launchpad.net/~lifeless/python-oops/fallbacks/+merge/78911
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops/fallbacks into lp:python-oops.

=== modified file 'NEWS'
--- NEWS	2011-10-04 07:10:04 +0000
+++ NEWS	2011-10-11 05:06:25 +0000
@@ -6,6 +6,17 @@
 NEXT
 ----
 
+0.0.9
+-----
+
+* Do not propogate oops ids of None to the report: they indicate that the oops
+  was not handled. (Robert Collins)
+
+* New helper function ``publish_new_only`` which lets you easily add fallback
+  publishers: if the first publisher doesn't and returns None,
+  ``publish_new_only`` will call the publisher it wraps. Otherwise it will just
+  return None itself. (Robert Collins)
+
 0.0.8
 -----
 

=== modified file 'oops/__init__.py'
--- oops/__init__.py	2011-10-04 07:13:08 +0000
+++ oops/__init__.py	2011-10-11 05:06:25 +0000
@@ -25,10 +25,12 @@
 # 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, 8, 'beta', 0)
+__version__ = (0, 0, 9, 'beta', 0)
 
 __all__ = [
-    'Config'
+    'Config',
+    'publish_new_only',
     ]
 
 from oops.config import Config
+from oops.publishers import publish_new_only

=== modified file 'oops/config.py'
--- oops/config.py	2011-10-04 07:20:18 +0000
+++ oops/config.py	2011-10-11 05:06:25 +0000
@@ -153,10 +153,25 @@
         automatically put into the report for them. All of the ids are also
         returned to the caller, allowing them to handle the case when multiple
         publishers allocated different ids. The id from the last publisher is
-        left in the report's id key as a convenience for the common case when
-        only one publisher is present.
+        left in the report's id key. This is done for three reasons:
+        
+        * As a convenience for the common case when only one publisher is
+          present.
+          
+        * This allows the next publisher in the chain to reuse that id if it
+          wishes.
+          
+        * Showing users that a report was made only needs one id and this means
+          other code doesn't need to arbitrarily pick one - it is done by
+          publish().
 
         If a publisher raises an exception, that will propagate to the caller.
+        If a publisher returns None, it indicates that the publisher did not
+        publish the report and that will not be stored in the reports 'id' key.
+        This permits fallback chains. For instance the first publisher may
+        choose not to publish a report (e.g. the place it would publish to is
+        offline) and the second publisher can detect that by checking if there
+        is an id key.
 
         :return: A list of the allocated ids.
         """
@@ -166,6 +181,7 @@
         result = []
         for publisher in self.publishers:
             id = publisher(report)
-            report['id'] = id
+            if id:
+                report['id'] = id
             result.append(id)
         return result

=== added file 'oops/publishers.py'
--- oops/publishers.py	1970-01-01 00:00:00 +0000
+++ oops/publishers.py	2011-10-11 05:06:25 +0000
@@ -0,0 +1,40 @@
+# Copyright (c) 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Generic publisher support and utility code."""
+
+__metaclass__ = type
+
+__all__ = [
+    'publish_new_only',
+    ]
+
+def publish_new_only(publisher):
+    """Wraps a publisher with a check that the report has not had an id set.
+    
+    This permits having fallback publishers that only publish if the earlier
+    one failed.
+
+    For instance:
+
+      >>> config.publishers.append(amqp_publisher)
+      >>> config.publishers.append(publish_new_only(datedir_repo.publish))
+    """
+    def result(report):
+        if report.get('id'):
+            return None
+        return publisher(report)
+    return result

=== modified file 'oops/tests/__init__.py'
--- oops/tests/__init__.py	2011-08-16 23:51:58 +0000
+++ oops/tests/__init__.py	2011-10-11 05:06:25 +0000
@@ -23,6 +23,7 @@
     test_mod_names = [
         'config',
         'createhooks',
+        'publishers',
         ]
     return TestLoader().loadTestsFromNames(
         ['oops.tests.test_' + name for name in test_mod_names])

=== modified file 'oops/tests/test_config.py'
--- oops/tests/test_config.py	2011-08-17 01:07:34 +0000
+++ oops/tests/test_config.py	2011-10-11 05:06:25 +0000
@@ -100,3 +100,20 @@
         report = {}
         self.assertEqual(None, config.publish(report))
         self.assertEqual(['ok', 'block'], calls)
+
+    def test_publishers_returning_None_no_change_to_report(self):
+        # If a publisher returns None (indicating it did not publish) no change
+        # is made to the report - this permits chaining publishers as a primary
+        # and fallback: The fallback can choose to do nothing if there is an id
+        # already assigned (and returns None to signal what it did); the caller
+        # Sees the actual assigned id in the report
+        def pub_succeed(report):
+            return '1'
+        def pub_noop(report):
+            return None
+        config = Config()
+        config.publishers.append(pub_succeed)
+        config.publishers.append(pub_noop)
+        report = {}
+        self.assertEqual(['1', None], config.publish(report))
+        self.assertEqual({'id': '1'}, report)

=== added file 'oops/tests/test_publishers.py'
--- oops/tests/test_publishers.py	1970-01-01 00:00:00 +0000
+++ oops/tests/test_publishers.py	2011-10-11 05:06:25 +0000
@@ -0,0 +1,38 @@
+# Copyright (c) 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the publishers module."""
+
+__metaclass__ = type
+
+import testtools
+from testtools.matchers import Is
+
+from oops import publish_new_only
+
+class TestPublishers(testtools.TestCase):
+
+    def test_publish_new_only_id_in_report(self):
+        def pub_fail(report):
+            self.fail('publication called')
+        publisher = publish_new_only(pub_fail)
+        publisher({'id': 'foo'})
+
+    def test_publish_new_only_no_id_in_report(self):
+        calls = []
+        publisher = publish_new_only(calls.append)
+        publisher({'foo': 'bar'})
+        self.assertEqual([{'foo': 'bar'}], calls)

=== modified file 'setup.py'
--- setup.py	2011-10-04 07:13:08 +0000
+++ setup.py	2011-10-11 05:06:25 +0000
@@ -23,7 +23,7 @@
         os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
 
 setup(name="oops",
-      version="0.0.8",
+      version="0.0.9",
       description=\
               "OOPS report model and default allocation/[de]serialization.",
       long_description=description,