launchpad-reviewers team mailing list archive

[Ivo Kracht <ivo.kracht@xxxxxx>]

Ivo Kracht has proposed merging lp:~ivo-kracht/launchpad/bug-728129 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #728129 in Launchpad itself: "decoratedresultset does not invoke pre_iter_hook for .any, .first etc"
  https://bugs.launchpad.net/launchpad/+bug/728129

For more details, see:
https://code.launchpad.net/~ivo-kracht/launchpad/bug-728129/+merge/113612

I added the pre_iter_hook to the methods (first(), last(), any(), one()) as well as adding a new test file to make the original test work properly. Finally I adjusted the original test to fit the new conditions and added a test for the case that a method returns None.

Pre-imp call with adeuring

test:
./bin/test services -vvt decoratedresultset

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/database/decoratedresultset.py
  lib/lp/services/database/tests/decoratedresultset.txt
  lib/lp/services/database/tests/test_decoratedresultset.py

./lib/lp/services/database/tests/decoratedresultset.txt
       1: narrative uses a moin header.
      36: narrative uses a moin header.
      60: narrative uses a moin header.
      78: narrative uses a moin header.
      86: narrative uses a moin header.
      96: narrative uses a moin header.
     113: narrative uses a moin header.
     122: narrative uses a moin header.
     137: narrative uses a moin header.
     149: narrative uses a moin header.
     156: narrative uses a moin header.
     173: narrative uses a moin header.

Since it is nearly end of the work day i didn't have time to fix these errors
-- 
https://code.launchpad.net/~ivo-kracht/launchpad/bug-728129/+merge/113612
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~ivo-kracht/launchpad/bug-728129 into lp:launchpad.

=== modified file 'lib/lp/services/database/decoratedresultset.py'
--- lib/lp/services/database/decoratedresultset.py	2012-03-27 00:15:07 +0000
+++ lib/lp/services/database/decoratedresultset.py	2012-07-05 17:15:27 +0000
@@ -167,12 +167,17 @@
         else:
             return self.decorate_or_none(value)
 
+    def iterhook_one_elem(self, value):
+        if value is not None and self.pre_iter_hook is not None:
+            self.pre_iter_hook([value])
+
     def any(self, *args, **kwargs):
         """See `IResultSet`.
 
         :return: The decorated version of the returned value.
         """
         value = self.result_set.any(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def first(self, *args, **kwargs):
@@ -181,6 +186,7 @@
         :return: The decorated version of the returned value.
         """
         value = self.result_set.first(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def last(self, *args, **kwargs):
@@ -189,6 +195,7 @@
         :return: The decorated version of the returned value.
         """
         value = self.result_set.last(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def one(self, *args, **kwargs):
@@ -197,6 +204,7 @@
         :return: The decorated version of the returned value.
         """
         value = self.result_set.one(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def order_by(self, *args, **kwargs):

=== modified file 'lib/lp/services/database/tests/decoratedresultset.txt'
--- lib/lp/services/database/tests/decoratedresultset.txt	2011-12-20 08:55:34 +0000
+++ lib/lp/services/database/tests/decoratedresultset.txt	2012-07-05 17:15:27 +0000
@@ -25,10 +25,13 @@
     >>> def result_decorator(distribution):
     ...     return "Dist name is: %s" % distribution.name
 
+    >>> def pre_iter_hook(values):
+    ...     print len(values), "elements in result set"
+
     >>> from lp.services.database.decoratedresultset import (
     ...     DecoratedResultSet)
     >>> decorated_result_set = DecoratedResultSet(
-    ...     proxied_result_set, result_decorator)
+    ...     proxied_result_set, result_decorator, pre_iter_hook)
 
 == copy() ==
 
@@ -49,6 +52,7 @@
 
     >>> for distro in result_copy:
     ...     distro
+    7 elements in result set
     u'Dist name is: debian'
     ...
     u'Dist name is: ubuntutest'
@@ -86,6 +90,7 @@
 
     >>> orig_result = decorated_result_set.result_set.any()
     >>> decorated_result_set.any() == result_decorator(orig_result)
+    1 elements in result set
     True
 
 == first() ==
@@ -94,14 +99,24 @@
 method and decorates the result:
 
     >>> decorated_result_set.first()
+    1 elements in result set
     u'Dist name is: debian'
 
+pre_iter_hook is not called from methods like first() or one() which return
+at most one row:
+
+    >>> empty_result_set = decorated_result_set.copy()
+    >>> print empty_result_set.config(
+    ...     offset=empty_result_set.count()).first()
+    None
+
 == last() ==
 
 The decorated last() method calls the original result set's last()
 method and decorates the result:
 
     >>> decorated_result_set.last()
+    1 elements in result set
     u'Dist name is: ubuntutest'
 
 == order_by() ==
@@ -114,6 +129,7 @@
     ...     Desc(Distribution.name))
     >>> for dist in ordered_results:
     ...     dist
+    7 elements in result set
     u'Dist name is: ubuntutest'
     ...
     u'Dist name is: debian'
@@ -124,6 +140,7 @@
 method and decorates the result:
 
     >>> decorated_result_set.config(offset=2, limit=1).one()
+    1 elements in result set
     u'Dist name is: redhat'
 
     >>> result_decorator(decorated_result_set.result_set.one())

=== added file 'lib/lp/services/database/tests/test_decoratedresultset.py'
--- lib/lp/services/database/tests/test_decoratedresultset.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/database/tests/test_decoratedresultset.py	2012-07-05 17:15:27 +0000
@@ -0,0 +1,36 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test harness for running the buglinktarget.txt interface test
+
+This module will run the interface test against the CVE, Specification and
+Question implementations of that interface.
+"""
+
+__metaclass__ = type
+
+__all__ = []
+
+import unittest
+
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.systemdocs import (
+    LayeredDocFileSuite,
+    setUp,
+    tearDown,
+    )
+
+
+def test_suite():
+    suite = unittest.TestSuite()
+
+    test = LayeredDocFileSuite(
+        'decoratedresultset.txt',
+        setUp=setUp, tearDown=tearDown,
+        layer=DatabaseFunctionalLayer)
+    suite.addTest(test)
+    return suite
+
+
+if __name__ == '__main__':
+    unittest.main()