launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/orderingcheck-unicode into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/launchpad/orderingcheck-unicode/+merge/80987

= Summary =

OrderingCheck is a helper class that verifies an expected ordering of a sequence without forcing you to listify or otherwise evaluate it.  It fits comfortably in your existing loop.

I just noticed however that it used str() to obtain human-readable representations of objects.  That's going to break with non-ASCII text.


== Proposed fix ==

Replace str() with repr().


== Pre-implementation notes ==

None.  I just happened to have use for this helper in my work for bug 884649.


== Implementation details ==

It takes a while for the test to start up, thanks to all the layers it drags along without need.  I was tempted to replace it with a unit test, but it's actually quite usable as documentation and at the same time pretty complete.

So all I did was touch the doctest very lightly.  It does exercise my code change, but without a change in output.  I didn't bother adding a test for non-ASCII text; this is an assertion message after all, and nothing depends on its exact contents.


== Tests ==

{{{
./bin/test -vvc canonical.launchpad -t orderingcheck
}}}


== Demo and Q/A ==

There is no Q/A to do, really, since exercising my code change would involve triggering an assertion error that we're not supposed to have in the first place.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/utilities/orderingcheck.py
  lib/canonical/launchpad/doc/orderingcheck.txt
-- 
https://code.launchpad.net/~jtv/launchpad/orderingcheck-unicode/+merge/80987
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/orderingcheck-unicode into lp:launchpad.

=== modified file 'lib/canonical/launchpad/doc/orderingcheck.txt'
--- lib/canonical/launchpad/doc/orderingcheck.txt	2009-05-07 12:04:41 +0000
+++ lib/canonical/launchpad/doc/orderingcheck.txt	2011-11-02 08:12:24 +0000
@@ -1,4 +1,5 @@
-= OrderingCheck =
+OrderingCheck
+=============
 
 Often when you iterate over a sequence of items, you assume that they're
 in some particular order.  If you're worried that they might not be in
@@ -22,10 +23,11 @@
     ...     checker.check(number)
 
 
-== Sorting criteria ==
+Sorting criteria
+----------------
 
-Like the built-in Python ordering functions, you can also use a
-comparison function instead of a key function and/or reverse the order.
+The OrderingCheck accepts all the same sorting options (as keyword args)
+as Python's built-in sorting functions.
 
     >>> def sort_cmp(left_item, right_item):
     ...     return left_item - right_item
@@ -35,7 +37,8 @@
     ...     checker.check(number)
 
 
-== Unexpected values ==
+Unexpected values
+-----------------
 
 If any item is out of sequence, the OrderingCheck raises an assertion
 error.
@@ -48,7 +51,8 @@
     AssertionError: Unexpected ordering at item 1: 0 should come before 1.
 
 
-== Edge cases ==
+Edge cases
+----------
 
 It is safe to use the None value.  Python places it below any other
 integer.
@@ -66,10 +70,11 @@
     >>> checker.check(2)
 
 
-== Customization ==
+Customization
+-------------
 
-If raising an assertion error is not the right thing to do when an
-incorrect ordering is seen, override the "fail" method.
+If raising an assertion error is not the response you want for a bad
+ordering, override the "fail" method.
 
     >>> def alternative_fail(item):
     ...     """Don't raise an error, just print a message."""
@@ -84,10 +89,11 @@
     >>> checker.check(8)
     Item 8 was out of sequence.
 
-The failure handler allowed execution to continue despite the unexpected
-value.  The ordering check continues on the assumption that it was an
-acceptable value after all, and takes it as the comparison base for the
-next value.
+Because this custom failure handler did not raise an error, execution
+continued despite the unexpected values.
+
+Since no exception was raised, the OrderingCheck still accepted that
+last value as a comparison base for the next one.
 
     >>> checker.check(9)
 

=== modified file 'lib/canonical/launchpad/utilities/orderingcheck.py'
--- lib/canonical/launchpad/utilities/orderingcheck.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/utilities/orderingcheck.py	2011-11-02 08:12:24 +0000
@@ -1,17 +1,20 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
-__all__ = ['OrderingCheck']
+__all__ = [
+    'OrderingCheck',
+    ]
 
 
 class OrderingCheck:
     """Helper class: verify that items are in an expected order.
 
     Use this if to verify that a series of items you are iterating over
-    is in some expected order.  If any of the items is not where you
-    expected it, an error is raised.
+    is in some expected order.  Any items that are not ordered the way
+    you expect are reported to a customizable failure handler; it raises
+    an error by default.
     """
     def __init__(self, **kwargs):
         """Define an ordering.  Parameters are as for sorted()."""
@@ -43,4 +46,4 @@
         """
         raise AssertionError(
             "Unexpected ordering at item %d: %s should come before %s." % (
-                self.item_count, str(item), str(self.last_item)))
+                self.item_count, repr(item), repr(self.last_item)))