launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-frozenset-subclassing into launchpad:master.

Commit message:
Fix frozenset subclassing for Python 3

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/385711

As of Python 3.0, following the fix for https://bugs.python.org/issue1721812, operations on subclasses of frozenset that create new sets do so using the base type, not the subclass type.  Refactor the custom sets in lp.bugs.model.bug to cope with this.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-frozenset-subclassing into launchpad:master.

diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index 98169c6..0a9062a 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -29,6 +29,10 @@ import re
 from lazr.lifecycle.event import ObjectCreatedEvent
 from lazr.lifecycle.snapshot import Snapshot
 import pytz
+from six.moves.collections_abc import (
+    Iterable,
+    Set,
+    )
 from sqlobject import (
     BoolCol,
     ForeignKey,
@@ -2259,8 +2263,61 @@ def load_people(*where):
         need_preferred_email=True)
 
 
-class BugSubscriberSet(frozenset):
-    """A set of bug subscribers
+class FrozenSetBasedSet(Set):
+    """A subclassable immutable set.
+
+    On Python 3, we can't simply subclass `frozenset`: set operations such
+    as union will create a new set, but it will be a plain `frozenset` and
+    will lack the appropriate custom properties.  However, the `Set` ABC
+    (which is in fact an immutable set; `MutableSet` is a separate ABC)
+    knows to create new sets using the same class.  Take advantage of this
+    with a trivial implementation of `Set` that backs straight onto a
+    `frozenset`.
+
+    The ABC doesn't implement the non-operator versions of set methods such
+    as `union`, so do that here, at least for those methods we actually use.
+    """
+
+    def __init__(self, iterable=None):
+        self._frozenset = (
+            frozenset() if iterable is None else frozenset(iterable))
+
+    def __iter__(self):
+        return iter(self._frozenset)
+
+    def __contains__(self, value):
+        return value in self._frozenset
+
+    def __len__(self):
+        return len(self._frozenset)
+
+    def issubset(self, other):
+        return self <= self._from_iterable(other)
+
+    def issuperset(self, other):
+        return self >= self._from_iterable(other)
+
+    def union(self, *others):
+        for other in others:
+            if not isinstance(other, Iterable):
+                raise NotImplementedError
+        return self._from_iterable(value for value in chain(self, *others))
+
+    def intersection(self, *others):
+        for other in others:
+            if not isinstance(other, Iterable):
+                raise NotImplementedError
+        return self._from_iterable(
+            value for value in chain(*others) if value in self)
+
+    def difference(self, *others):
+        other = self._from_iterable([]).union(*others)
+        return self._from_iterable(
+            value for value in self if value not in other)
+
+
+class BugSubscriberSet(FrozenSetBasedSet):
+    """An immutable set of bug subscribers.
 
     Every member should provide `IPerson`.
     """
@@ -2274,8 +2331,8 @@ class BugSubscriberSet(frozenset):
         return tuple(sorted(self, key=person_sort_key))
 
 
-class BugSubscriptionSet(frozenset):
-    """A set of bug subscriptions."""
+class BugSubscriptionSet(FrozenSetBasedSet):
+    """An immutable set of bug subscriptions."""
 
     @cachedproperty
     def sorted(self):
@@ -2299,7 +2356,7 @@ class BugSubscriptionSet(frozenset):
             return BugSubscriberSet(load_people(condition))
 
 
-class StructuralSubscriptionSet(frozenset):
+class StructuralSubscriptionSet(FrozenSetBasedSet):
     """A set of structural subscriptions."""
 
     @cachedproperty
diff --git a/lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py b/lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py
index f261fcb..c9565aa 100644
--- a/lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py
+++ b/lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py
@@ -8,6 +8,7 @@ __metaclass__ = type
 from contextlib import contextmanager
 
 import six
+from six.moves.collections_abc import Set
 from storm.store import Store
 from testtools.matchers import Equals
 from zope.component import queryAdapter
@@ -71,7 +72,7 @@ class TestSubscriptionRelatedSets(TestCaseWithFactory):
 
     def test_BugSubscriberSet(self):
         subscriber_set = BugSubscriberSet(self.subscribers_set)
-        self.assertIsInstance(subscriber_set, frozenset)
+        self.assertIsInstance(subscriber_set, Set)
         self.assertEqual(self.subscribers_set, subscriber_set)
         self.assertEqual(self.subscribers_sorted, subscriber_set.sorted)
 
@@ -82,7 +83,7 @@ class TestSubscriptionRelatedSets(TestCaseWithFactory):
                 bug.subscribe(subscriber, subscriber)
                 for subscriber in self.subscribers_set)
         subscription_set = BugSubscriptionSet(subscriptions)
-        self.assertIsInstance(subscription_set, frozenset)
+        self.assertIsInstance(subscription_set, Set)
         self.assertEqual(subscriptions, subscription_set)
         # BugSubscriptionSet.sorted returns a tuple of subscriptions ordered
         # by subscribers.
@@ -102,7 +103,7 @@ class TestSubscriptionRelatedSets(TestCaseWithFactory):
                 product.addSubscription(subscriber, subscriber)
                 for subscriber in self.subscribers_set)
         subscription_set = StructuralSubscriptionSet(subscriptions)
-        self.assertIsInstance(subscription_set, frozenset)
+        self.assertIsInstance(subscription_set, Set)
         self.assertEqual(subscriptions, subscription_set)
         # StructuralSubscriptionSet.sorted returns a tuple of subscriptions
         # ordered by subscribers.