Merge lp:~leonardr/lazr.batchnavigator/build-url-without-length into lp:lazr.batchnavigator

[Leonard Richardson]

A previous branch changed the Batch object to avoid calling len() unless it was absolutely necessary, since len() can be inefficient. This branch changes the BatchNavigator object to avoid calling len() unless absolutely necessary. (The places where it's absolutely necessary: getting the link to the final batch, and determining whether the header string is plural or not. The plural determination could probably be optimized further: whenever any batch data is present, we know whether there's one item in the batch or whether there's more. But this isn't what I'm trying to optimize just now.)

I use the same trick to test this as Benji used to test his changes to the Batch object. I define a simple subclass of the 'list' type that raises a RuntimeError when __len__ is called, and use it throughout the existing tests (except where noted).

[lp:~leonardr/lazr.batchnavigator/build-url-without-length]

37. By Leonard Richardson on 2010-08-19

Give up trying to optimize .heading after realizing that it doesn't matter, since .heading is always invoked immediately after an explicit determination of length.

38. By Leonard Richardson on 2010-08-19

Updated NEWS.

[Guilherme Salgado (salgado)]

This looks very nice

review: Approve (code)

=== modified file 'src/lazr/batchnavigator/NEWS.txt'
--- src/lazr/batchnavigator/NEWS.txt	2010-08-18 11:38:56 +0000
+++ src/lazr/batchnavigator/NEWS.txt	2010-08-19 14:26:43 +0000
@@ -2,12 +2,14 @@
 NEWS for lazr.batchnavigator
 ============================
 
-1.2.2 (Unreleased)
+1.2.2 (2010-08-19)
 ==================
 
 - Make len() cheap to call when the current batch is the last (or
   only) batch.
 
+- Avoid calling len() when generating navigator URLs.
+
 1.2.1 (2010-08-12)
 ==================
 

=== modified file 'src/lazr/batchnavigator/README.txt'
--- src/lazr/batchnavigator/README.txt	2010-07-21 19:49:40 +0000
+++ src/lazr/batchnavigator/README.txt	2010-08-19 14:26:43 +0000
@@ -64,15 +64,38 @@
 
 You can create a batch and a batch navigator like so:
 
+    >>> safe_reindeer = reindeer
+    >>> safe_reindeer_batch_navigator = BatchNavigator(
+    ...     safe_reindeer, build_request(), size=3)
+
+An important feature of lazr.batchnavigator is its reluctance to
+invoke len() on an underlying data set. len() can be an expensive
+operation that provides little benefit, so this library tries hard to
+avoid calling len() unless it's absolutely necessary. To show this
+off, we'll define a subclass of Python's list type that explodes when
+len() is invoked on it.
+
+    >>> class ListWithExplosiveLen(list):
+    ...     """A list subclass that doesn't like its len() being called."""
+    ...     def __len__(self):
+    ...         raise RuntimeError
+
+Unless otherwise stated, we will use this list exclusively throughout
+this test, to verify that len() is never called unless we want it to
+be.
+
+    >>> explosive_reindeer = ListWithExplosiveLen(reindeer)
     >>> reindeer_batch_navigator = BatchNavigator(
-    ...     reindeer, build_request(), size=3)
+    ...     explosive_reindeer, build_request(), size=3)
 
-The BatchNavigator implements IBatchNavigator.
+The BatchNavigator implements IBatchNavigator. We need to use the
+'safe' batch navigator here, because verifyObject probes all methods
+of the object it's passed, including __len__.
 
     >>> from zope.interface.verify import verifyObject
     >>> from lazr.batchnavigator.interfaces import IBatchNavigator
 
-    >>> verifyObject(IBatchNavigator, reindeer_batch_navigator)
+    >>> verifyObject(IBatchNavigator, safe_reindeer_batch_navigator)
     True
 
 The BatchNavigator class provides IBatchNavigatorFactory.  This can be used
@@ -97,7 +120,12 @@
     ''
     >>> reindeer_batch_navigator.nextBatchURL()
     'http://www.example.com/foo?start=3'
-    >>> reindeer_batch_navigator.lastBatchURL()
+
+There's no way to get the URL to the final batch without knowing the
+length of the entire list, so we'll use the safe batch navigator to
+demonstrate lastBatchURL():
+
+    >>> safe_reindeer_batch_navigator.lastBatchURL()
     'http://www.example.com/foo?start=6'
 
 The next link will be empty when there are no further results:
@@ -130,6 +158,9 @@
     ...                          'batch': '3'})
     >>> reindeer_batch_navigator_with_qs = BatchNavigator(
     ...     reindeer, request, size=3)
+    >>> safe_reindeer_batch_navigator_with_qs = BatchNavigator(
+    ...     safe_reindeer, request, size=3)
+
 
 In this case, we created the BatchNavigator with a default size of '3' and
 the request is asking exactly that number of items per batch, and thus, we
@@ -141,12 +172,16 @@
     'http://www.example.com/foo?fnorb=bar&start=0'
     >>> reindeer_batch_navigator_with_qs.nextBatchURL()
     'http://www.example.com/foo?fnorb=bar&start=6'
-    >>> reindeer_batch_navigator_with_qs.lastBatchURL()
+
+(Again, there's no way to get the last batch without knowing the size
+of the entire list.)
+
+    >>> safe_reindeer_batch_navigator_with_qs.lastBatchURL()
     'http://www.example.com/foo?fnorb=bar&start=6'
 
 You can ask for the links for each of the result pages:
 
-    >>> links = reindeer_batch_navigator.batchPageURLs()
+    >>> links = safe_reindeer_batch_navigator.batchPageURLs()
     >>> for link in links:
     ...     label, url = link.items()[0]
     ...     print label, url
@@ -281,7 +316,7 @@
     ...
     >>> sm = getSiteManager()
     >>> sm.registerAdapter(ExampleAdapter)
-    >>> example = ExampleResultSet(reindeer)
+    >>> example = ExampleResultSet(safe_reindeer)
     >>> example_batch_navigator = BatchNavigator(
     ...     example, build_request(), size=3)
     >>> example_batch_navigator.currentBatch().total()
@@ -417,16 +452,19 @@
 parameter called "absent," but it's not passed in our ongoing page
 request.
 
-    >>> navigator_with_parameters = BatchNavigator(
-    ...     reindeer, request_with_parameters, size=3,
-    ...     transient_parameters=['quiet', 'absent'])
+    >>> def build_navigator(list):
+    ...     return BatchNavigator(
+    ...         list, request_with_parameters, size=3,
+    ...         transient_parameters=['quiet', 'absent'])
+    >>> navigator_with_parameters = build_navigator(reindeer)
+    >>> safe_navigator_with_parameters = build_navigator(safe_reindeer)
 
 Of these three parameters, only "noisy" recurs in the links produced by
 the batch navigator.
 
     >>> navigator_with_parameters.nextBatchURL()
     'http://www.example.com/foo?noisy=HELLO&start=3'
-    >>> navigator_with_parameters.lastBatchURL()
+    >>> safe_navigator_with_parameters.lastBatchURL()
     'http://www.example.com/foo?noisy=HELLO&start=6'
 
 The transient parameter is omitted, and the one that was never passed in
@@ -440,7 +478,7 @@
 BatchNavigator's heading property contains a description of the objects
 for display.
 
-    >>> navigator.heading
+    >>> safe_reindeer_batch_navigator.heading
     'results'
 
 There is a special case for when there is only one item in the batch,
@@ -450,11 +488,18 @@
     >>> navigator.heading
     'result'
 
+(Accessing .heading causes len() to be called on the underlying list,
+which is why we have to use the safe batch navigator. In theory, this
+could be optimized, but there's no real point, since the heading is
+invariably preceded by the actual length of the underlying list,
+eg. "10 results". Since len() is called anyway, and its value is
+cached, a second len() won't hurt performance.)
+
 The heading can be set by passing a singular and a plural version of
 the heading. The batch navigation will return the appropriate
 header based on the total items in the batch.
 
-    >>> navigator = BatchNavigator(reindeer, request=request)
+    >>> navigator = BatchNavigator(safe_reindeer, request=request)
     >>> navigator.setHeadings('bug', 'bugs')
     >>> navigator.heading
     'bugs'

=== modified file 'src/lazr/batchnavigator/_batchnavigator.py'
--- src/lazr/batchnavigator/_batchnavigator.py	2010-05-05 03:36:31 +0000
+++ src/lazr/batchnavigator/_batchnavigator.py	2010-08-19 14:26:43 +0000
@@ -185,7 +185,7 @@
 
     def generateBatchURL(self, batch):
         url = ""
-        if not batch:
+        if batch is None:
             return url
 
         qs = self.getCleanQueryString()

=== modified file 'src/lazr/batchnavigator/z3batching/batch.py'
--- src/lazr/batchnavigator/z3batching/batch.py	2010-08-18 11:38:56 +0000
+++ src/lazr/batchnavigator/z3batching/batch.py	2010-08-19 14:26:43 +0000
@@ -196,7 +196,7 @@
 
     def firstBatch(self):
         return _Batch(self.list, 0, size=self.size,
-                      _listlength=self.listlength)
+                      _listlength=self._listlength)
 
     def lastBatch(self):
         # Return the last possible batch for this dataset, at the