Merge ~cjwatson/launchpad:py3-orderable into launchpad:master

[Colin Watson]

Fix sorting of unorderable objects on Python 3

On Python 3, objects without a meaningful natural ordering can't be ordered. Fix a number of places that were trying to do this: sometimes this involves using better sort keys, while sometimes we can use various strategies to avoid ordering them in the first place.

[Cristian Gonzalez (cristiangsp)]

Looks good!

review: Approve

diff --git a/lib/lp/bugs/browser/bugtask.py b/lib/lp/bugs/browser/bugtask.py
index a9d9953..2a139c0 100644
--- a/lib/lp/bugs/browser/bugtask.py
+++ b/lib/lp/bugs/browser/bugtask.py
@@ -690,10 +690,14 @@ class BugTaskView(LaunchpadView, BugViewMixin, FeedsMixin):
         """
         event_groups = self._event_groups
 
+        def activity_sort_key(activity):
+            target = activity.target
+            return (
+                activity.datechanged, 0 if target is None else 1, target,
+                activity.attribute)
+
         def group_activities_by_target(activities):
-            activities = sorted(
-                activities, key=attrgetter(
-                    "datechanged", "target", "attribute"))
+            activities = sorted(activities, key=activity_sort_key)
             return [
                 {"target": target, "activity": list(activity)}
                 for target, activity in groupby(
diff --git a/lib/lp/bugs/browser/tests/person-bug-views.txt b/lib/lp/bugs/browser/tests/person-bug-views.txt
index 439512d..5f1d5ab 100644
--- a/lib/lp/bugs/browser/tests/person-bug-views.txt
+++ b/lib/lp/bugs/browser/tests/person-bug-views.txt
@@ -356,7 +356,7 @@ particular bug (see bug 1357):
 
     >>> commented_bugtasks_view = create_view(no_priv, '+commentedbugs')
     >>> commented_bugs = list(commented_bugtasks_view.search().batch)
-    >>> [bugtask.bug.id for bugtask in sorted(commented_bugs)]
+    >>> [bugtask.bug.id for bugtask in commented_bugs]
     [1, 1, 1]
 
 
diff --git a/lib/lp/bugs/doc/externalbugtracker-sourceforge.txt b/lib/lp/bugs/doc/externalbugtracker-sourceforge.txt
index a01dbdc..bcaf316 100644
--- a/lib/lp/bugs/doc/externalbugtracker-sourceforge.txt
+++ b/lib/lp/bugs/doc/externalbugtracker-sourceforge.txt
@@ -215,9 +215,11 @@ been checked.
 
     >>> transaction.commit()
 
+    >>> from operator import attrgetter
     >>> with sourceforge.responses(trace_calls=True):
     ...     bug_watch_updater.updateBugWatches(
-    ...         sourceforge, sorted(example_bug_tracker.watches))
+    ...         sourceforge,
+    ...         sorted(example_bug_tracker.watches, key=attrgetter('id')))
     INFO Updating 1 watches for 1 bugs on http://bugs.some.where
     GET http://bugs.some.where/support/tracker.php?aid=1722251
 
diff --git a/lib/lp/bugs/model/tests/test_bugtask.py b/lib/lp/bugs/model/tests/test_bugtask.py
index 527120e..21a91e4 100644
--- a/lib/lp/bugs/model/tests/test_bugtask.py
+++ b/lib/lp/bugs/model/tests/test_bugtask.py
@@ -11,7 +11,11 @@ import unittest
 
 from lazr.lifecycle.snapshot import Snapshot
 from storm.store import Store
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    MatchesSetwise,
+    MatchesStructure,
+    )
 from testtools.testcase import ExpectedException
 import transaction
 from zope.component import getUtility
@@ -199,12 +203,11 @@ class TestBugTaskCreation(TestCaseWithFactory):
             bug_many, mark,
             [evolution, a_distro, warty],
             status=BugTaskStatus.FIXRELEASED)
-        tasks = [(t.product, t.distribution, t.distroseries) for t in taskset]
-        tasks.sort()
 
-        self.assertEqual(tasks[0][2], warty)
-        self.assertEqual(tasks[1][1], a_distro)
-        self.assertEqual(tasks[2][0], evolution)
+        self.assertThat(taskset, MatchesSetwise(
+            MatchesStructure.byEquality(product=evolution),
+            MatchesStructure.byEquality(distribution=a_distro),
+            MatchesStructure.byEquality(distroseries=warty)))
 
     def test_accesspolicyartifacts_updated(self):
         # createManyTasks updates the AccessPolicyArtifacts related
diff --git a/lib/lp/bugs/stories/webservice/xx-bug.txt b/lib/lp/bugs/stories/webservice/xx-bug.txt
index 04480a5..6121737 100644
--- a/lib/lp/bugs/stories/webservice/xx-bug.txt
+++ b/lib/lp/bugs/stories/webservice/xx-bug.txt
@@ -312,9 +312,11 @@ Bug tasks
 Each bug may be associated with one or more bug tasks. Much of the
 data in a bug task is derived from the bug.
 
+    >>> from operator import itemgetter
     >>> bug_one_bugtasks_url = bug_one['bug_tasks_collection_link']
     >>> bug_one_bugtasks = sorted(webservice.get(
-    ...     bug_one_bugtasks_url).jsonBody()['entries'])
+    ...     bug_one_bugtasks_url).jsonBody()['entries'],
+    ...     key=itemgetter('self_link'))
     >>> len(bug_one_bugtasks)
     3
 
@@ -825,7 +827,6 @@ Bug subscriptions
 
 We can get the collection of subscriptions to a bug.
 
-    >>> from operator import itemgetter
     >>> bug_one_subscriptions_url = bug_one['subscriptions_collection_link']
     >>> subscriptions = webservice.get(bug_one_subscriptions_url).jsonBody()
     >>> subscription_entries = sorted(
@@ -1056,7 +1057,8 @@ case aspects of the bugtask (like status) are slaved to the remote bug
 report described by the bugwatch.
 
     >>> bug_one_bug_watches = sorted(webservice.get(
-    ...     bug_one['bug_watches_collection_link']).jsonBody()['entries'])
+    ...     bug_one['bug_watches_collection_link']).jsonBody()['entries'],
+    ...     key=itemgetter('self_link'))
     >>> len(bug_one_bug_watches)
     4
 
diff --git a/lib/lp/bugs/subscribers/bug.py b/lib/lp/bugs/subscribers/bug.py
index 11851eb..d88d27b 100644
--- a/lib/lp/bugs/subscribers/bug.py
+++ b/lib/lp/bugs/subscribers/bug.py
@@ -15,6 +15,7 @@ __all__ = [
 
 
 import datetime
+from operator import attrgetter
 
 from zope.security.proxy import removeSecurityProxy
 
@@ -227,7 +228,7 @@ def send_bug_details_to_new_bug_subscribers(
     recipients = bug.getBugNotificationRecipients()
 
     bug_notification_builder = BugNotificationBuilder(bug, event_creator)
-    for to_person in sorted(to_persons):
+    for to_person in sorted(to_persons, key=attrgetter('id')):
         reason, rationale = recipients.getReason(
             str(removeSecurityProxy(to_person).preferredemail.email))
         subject, contents = generate_bug_add_email(
diff --git a/lib/lp/code/doc/revision.txt b/lib/lp/code/doc/revision.txt
index 46f0219..e514d69 100644
--- a/lib/lp/code/doc/revision.txt
+++ b/lib/lp/code/doc/revision.txt
@@ -112,7 +112,9 @@ sequence attribute is None.
     >>> ancestry = IStore(BranchRevision).find(
     ...     BranchRevision, BranchRevision.branch == branch)
     >>> for branch_revision in sorted(ancestry,
-    ...         key=lambda r:(r.sequence, r.revision.id), reverse=True):
+    ...         key=lambda r: (
+    ...             0 if r.sequence is None else 1, r.sequence, r.revision.id),
+    ...         reverse=True):
     ...     print(branch_revision.sequence, branch_revision.revision.id)
     6 9
     5 8
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 061fb1e..55a9197 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -1290,9 +1290,10 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
 
     def getRevisionsSinceReviewStart(self):
         """Get the grouped revisions since the review started."""
+        all_comments = list(self.all_comments)
         entries = [
-            ((comment.date_created, -1), comment) for comment
-            in self.all_comments]
+            ((comment.date_created, i - len(all_comments)), comment)
+            for i, comment in enumerate(all_comments)]
         entries.extend(self._getNewerRevisions())
         entries.sort()
         current_group = []
diff --git a/lib/lp/registry/stories/webservice/xx-person.txt b/lib/lp/registry/stories/webservice/xx-person.txt
index 73450d0..619784c 100644
--- a/lib/lp/registry/stories/webservice/xx-person.txt
+++ b/lib/lp/registry/stories/webservice/xx-person.txt
@@ -303,8 +303,10 @@ Team memberships are first-class objects with their own URLs.
 
 Team memberships also have data fields.
 
-    >>> salgado_landscape = sorted(webservice.get(
-    ...     salgado_memberships).jsonBody()['entries'])[1]
+    >>> salgado_landscape = [
+    ...     entry for entry in webservice.get(
+    ...         salgado_memberships).jsonBody()['entries']
+    ...     if entry['team_link'].endswith('~landscape-developers')][0]
     >>> for key in sorted(salgado_landscape):
     ...     print(key)
     date_expires
diff --git a/lib/lp/registry/tests/test_distroseriesdifferencecomment.py b/lib/lp/registry/tests/test_distroseriesdifferencecomment.py
index f95aac7..6276647 100644
--- a/lib/lp/registry/tests/test_distroseriesdifferencecomment.py
+++ b/lib/lp/registry/tests/test_distroseriesdifferencecomment.py
@@ -33,7 +33,7 @@ def get_comment_source():
 
 def randomize_list(original_list):
     """Sort a list (or other iterable) in random order.  Return list."""
-    return sorted(original_list, key=lambda _: random)
+    return sorted(original_list, key=lambda _: random())
 
 
 class DistroSeriesDifferenceCommentTestCase(TestCaseWithFactory):
diff --git a/lib/lp/services/doc/orderingcheck.txt b/lib/lp/services/doc/orderingcheck.txt
index 209b6e0..e2367aa 100644
--- a/lib/lp/services/doc/orderingcheck.txt
+++ b/lib/lp/services/doc/orderingcheck.txt
@@ -10,8 +10,12 @@ seem worth the trouble.
     >>> from lp.services.orderingcheck import OrderingCheck
 
     >>> def sort_key(item):
-    ...     """Simple sorting key for integers: the integer itself."""
-    ...     return item
+    ...     """Simple sorting key for integers.
+    ...
+    ...     This could just be the integer itself, but in order to support
+    ...     None on Python 3 we need an additional term.
+    ...     """
+    ...     return (0 if item is None else 1, item)
 
 The OrderingCheck makes it clean and easy.  You create an OrderingCheck
 with the same arguments that go into Python's standard sorting
@@ -51,7 +55,7 @@ error.
 Edge cases
 ----------
 
-It is safe to use the None value.  Python places it below any other
+It is safe to use the None value.  sort_key places it below any other
 integer.
 
     >>> checker = OrderingCheck(key=sort_key)
diff --git a/lib/lp/services/webservice/stories/multiversion.txt b/lib/lp/services/webservice/stories/multiversion.txt
index 22f9722..683325f 100644
--- a/lib/lp/services/webservice/stories/multiversion.txt
+++ b/lib/lp/services/webservice/stories/multiversion.txt
@@ -46,11 +46,14 @@ In the 'beta' version of the web service, mutator methods like
 IBugTask.transitionToStatus are published as named operations. In
 subsequent versions, those named operations are not published.
 
+  >>> from operator import itemgetter
+
   >>> def get_bugtask_path(version):
   ...     bug_one = webservice.get("/bugs/1", api_version=version).jsonBody()
   ...     bug_one_bugtasks_url = bug_one['bug_tasks_collection_link']
   ...     bug_one_bugtasks = sorted(webservice.get(
-  ...         bug_one_bugtasks_url).jsonBody()['entries'])
+  ...         bug_one_bugtasks_url).jsonBody()['entries'],
+  ...         key=itemgetter('self_link'))
   ...     bugtask_path = bug_one_bugtasks[0]['self_link']
   ...     return bugtask_path
 
diff --git a/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py b/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
index 0b82990..187323c 100644
--- a/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
+++ b/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
@@ -152,4 +152,6 @@ class TestScriptRunning(TestCaseWithFactory):
             sorted(
                 [(result.binary_package_release, result.archive, result.day,
                   result.country, result.count) for result in results],
-                 key=lambda r: (r[0].id, r[2], r[3].name if r[3] else None)))
+                 key=lambda r: (
+                     r[0].id, r[2],
+                     r[3] is not None, r[3].name if r[3] else None)))
diff --git a/lib/lp/soyuz/stories/webservice/xx-hasbuildrecords.txt b/lib/lp/soyuz/stories/webservice/xx-hasbuildrecords.txt
index 3246cca..4b3ca4e 100644
--- a/lib/lp/soyuz/stories/webservice/xx-hasbuildrecords.txt
+++ b/lib/lp/soyuz/stories/webservice/xx-hasbuildrecords.txt
@@ -42,8 +42,10 @@ Celso Providelo PPA builds can be browsed via the API.
 
 An entry can be selected in the returned collection.
 
+    >>> from operator import itemgetter
     >>> from lazr.restful.testing.webservice import pprint_entry
-    >>> pprint_entry(sorted(ppa_builds['entries'])[0])
+    >>> pprint_entry(
+    ...     sorted(ppa_builds['entries'], key=itemgetter('title'))[0])
     arch_tag: 'hppa'
     ...
     title: 'hppa build of mozilla-firefox 0.9 in ubuntu warty RELEASE'
diff --git a/lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt b/lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt
index 761b7aa..92e2764 100644
--- a/lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt
+++ b/lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt
@@ -292,7 +292,7 @@ binaries have been copied and published in the same context archive.
     >>> source_pub = pubs['entries'][0]
     >>> builds = webservice.named_get(
     ...     source_pub['self_link'], 'getBuilds').jsonBody()
-    >>> for entry in sorted(builds['entries']):
+    >>> for entry in builds['entries']:
     ...     print(entry['title'])
     i386 build of pmount 0.1-1 in ubuntu warty RELEASE
 
@@ -310,7 +310,7 @@ of that publication.
     >>> source_pub = pubs['entries'][0]
     >>> builds = webservice.named_get(
     ...     source_pub['self_link'], 'getPublishedBinaries').jsonBody()
-    >>> for entry in sorted(builds['entries']):
+    >>> for entry in builds['entries']:
     ...     print(entry['display_name'])
     pmount 0.1-1 in warty hppa
     pmount 0.1-1 in warty i386
diff --git a/lib/lp/translations/utilities/translationmerger.py b/lib/lp/translations/utilities/translationmerger.py
index 192c32b..7164395 100644
--- a/lib/lp/translations/utilities/translationmerger.py
+++ b/lib/lp/translations/utilities/translationmerger.py
@@ -282,8 +282,16 @@ class MessageSharingMerge(LaunchpadScript):
         class_count = len(equivalence_classes)
         log.info("Merging %d template equivalence classes." % class_count)
 
+        def equivalence_class_sort_key(name):
+            template_name, package_name = name
+            if package_name is None:
+                return template_name, 0, None
+            else:
+                return template_name, 1, package_name
+
         tm = TransactionManager(self.txn, self.options.dry_run)
-        for number, name in enumerate(sorted(equivalence_classes)):
+        for number, name in enumerate(sorted(
+                equivalence_classes, key=equivalence_class_sort_key)):
             templates = equivalence_classes[name]
             log.info(
                 "Merging equivalence class '%s': %d template(s) (%d / %d)" % (