launchpad-reviewers team mailing list archive

[James Westby <jw+debian@xxxxxxxxxxxxxxx>]

James Westby has proposed merging lp:~james-w/python-oops/record-published-explicitly into lp:python-oops.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~james-w/python-oops/record-published-explicitly/+merge/109239

Hi,

In discussions around the fact that Django can't easily tell anyone
what the id is for the oops they just encountered Rob said that the
best way to handle it was probably to have django allocate the oops id.

That works great, and the publishers will just use it. The only drawback
is that it means that publish_only_new will never trigger its publisher
any more, because of the double meaning of the id in the report.

Applications that do this could avoid publish_only_new, at the cost of
duplicating all oopses and then de-duplicating them later.

Instead I propose to track the published state more explicitly in the
oops, which this branch does.

Thanks,

James
-- 
https://code.launchpad.net/~james-w/python-oops/record-published-explicitly/+merge/109239
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~james-w/python-oops/record-published-explicitly into lp:python-oops.

=== modified file 'oops/config.py'
--- oops/config.py	2011-11-13 21:24:42 +0000
+++ oops/config.py	2012-06-07 21:03:22 +0000
@@ -159,13 +159,13 @@
         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. 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().
@@ -189,6 +189,7 @@
             id = publisher(report)
             if id:
                 report['id'] = id
+                report['published'] = True
                 result.append(id)
             else:
                 result.append(None)

=== modified file 'oops/publishers.py'
--- oops/publishers.py	2011-11-13 21:24:42 +0000
+++ oops/publishers.py	2012-06-07 21:03:22 +0000
@@ -33,7 +33,7 @@
       >>> config.publishers.append(publish_new_only(datedir_repo.publish))
     """
     def result(report):
-        if report.get('id'):
+        if report.get('published'):
             return None
         return publisher(report)
     return result

=== modified file 'oops/tests/test_config.py'
--- oops/tests/test_config.py	2011-11-13 21:24:42 +0000
+++ oops/tests/test_config.py	2012-06-07 21:03:22 +0000
@@ -79,7 +79,7 @@
         config.publishers.append(pub_2)
         report = {}
         self.assertEqual(['1', '2'], config.publish(report))
-        self.assertEqual({'id': '2'}, report)
+        self.assertEqual('2', report['id'])
 
     def test_publish_filters_first(self):
         calls = []
@@ -101,7 +101,7 @@
         self.assertEqual(['ok', 'block'], calls)
 
     def test_publishers_returning_not_published_no_change_to_report(self):
-        # If a publisher returns non-zero (indicating it did not publish) no
+        # If a publisher returns a falseish value (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);
@@ -115,4 +115,26 @@
         config.publishers.append(pub_noop)
         report = {}
         self.assertEqual(['1', None], config.publish(report))
-        self.assertEqual({'id': '1'}, report)
+        self.assertEqual({'id': '1', 'published': True}, report)
+
+    def test_publisher_returning_not_published_doesnt_set_published(self):
+        # If a publisher indicates that it didn't publish then the
+        # 'published' key is not set in the report
+        def pub_noop(report):
+            return ''
+        config = Config()
+        config.publishers.append(pub_noop)
+        report = {}
+        config.publish(report)
+        self.assertEqual({}, report)
+
+    def test_publisher_returning_published_sets_published(self):
+        # If a publisher indicates that it did publish then the
+        # 'published' key is set in the report
+        def pub_succeed(report):
+            return '1'
+        config = Config()
+        config.publishers.append(pub_succeed)
+        report = {}
+        config.publish(report)
+        self.assertEqual({'id': '1', 'published': True}, report)

=== modified file 'oops/tests/test_publishers.py'
--- oops/tests/test_publishers.py	2011-11-13 21:24:42 +0000
+++ oops/tests/test_publishers.py	2012-06-07 21:03:22 +0000
@@ -18,20 +18,25 @@
 __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 test_publish_new_only_published_in_report(self):
         def pub_fail(report):
             self.fail('publication called')
         publisher = publish_new_only(pub_fail)
-        publisher({'id': 'foo'})
+        publisher({'published': True})
 
-    def test_publish_new_only_no_id_in_report(self):
+    def test_publish_new_only_no_published_in_report(self):
         calls = []
         publisher = publish_new_only(calls.append)
         publisher({'foo': 'bar'})
         self.assertEqual([{'foo': 'bar'}], calls)
+
+    def test_publish_new_only_no_published_false_in_report(self):
+        calls = []
+        publisher = publish_new_only(calls.append)
+        publisher({'published': False})
+        self.assertEqual([{'published': False}], calls)