[tor-commits] [bridgedb/master] Remove Bridges.BridgeHolder class.

isis at torproject.org isis at torproject.org
Sat Jul 25 19:26:22 UTC 2015

commit 346d321fa4f78626c49cd4ff6d82a346267f6c0c
Author: Isis Lovecruft <isis at torproject.org>
Date:   Sun Apr 19 03:25:11 2015 +0000

    Remove Bridges.BridgeHolder class.
    It wasn't a real metaclass, base class, or interface; its methods were
    never used; it didn't provide any decent documentation; not everything
    which inherited from it used its methods; nothing called super() for it,
    or otherwise called methods on it.  Totally useless.
     * REMOVE bridgedb.Bridges.BridgeHolder class.
     * CHANGE everything which previously inherited from BridgeHolder to
       simply inherit from object instead.
 lib/bridgedb/Bridges.py        |   63 ++++++++++++++++------------------------
 lib/bridgedb/Main.py           |    4 +--
 lib/bridgedb/persistent.py     |    3 +-
 lib/bridgedb/test/test_Main.py |    6 ++--
 4 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/lib/bridgedb/Bridges.py b/lib/bridgedb/Bridges.py
index 5742794..1a06dd8 100644
--- a/lib/bridgedb/Bridges.py
+++ b/lib/bridgedb/Bridges.py
@@ -39,18 +39,6 @@ DIGEST_LEN = 20
-class BridgeHolder(object):
-    """Abstract base class for all classes that hold bridges."""
-    def insert(self, bridge):
-        raise NotImplementedError
-    def clear(self):
-        pass
-    def dumpAssignments(self, f, description=""):
-        pass
 class BridgeRingParameters(object):
     """Store validated settings on minimum number of Bridges with certain
     attributes which should be included in any generated subring of a
@@ -99,7 +87,8 @@ class BridgeRingParameters(object):
         self.needPorts = needPorts[:]
         self.needFlags = [(flag.lower(), count) for flag, count in needFlags[:]]
-class BridgeRing(BridgeHolder):
+class BridgeRing(object):
     """Arranges bridges into a hashring based on an hmac function."""
     def __init__(self, key, answerParameters=None):
@@ -333,15 +322,13 @@ class BridgeRing(BridgeHolder):
             f.write("%s %s\n" % (b.fingerprint, " ".join(desc).strip()))
-class FixedBridgeSplitter(BridgeHolder):
-    """A bridgeholder that splits bridges up based on an hmac and assigns
-       them to several sub-bridgeholders with equal probability.
+class FixedBridgeSplitter(object):
+    """Splits bridges up based on an HMAC and assigns them to one of several
+    subhashrings with equal probability.
     def __init__(self, key, rings):
         self.hmac = getHMACFunc(key, hex=True)
         self.rings = rings[:]
-        for r in self.rings:
-            assert(isinstance(r, BridgeHolder))
     def insert(self, bridge):
         # Grab the first 4 bytes
@@ -366,7 +353,7 @@ class FixedBridgeSplitter(BridgeHolder):
         """Write all bridges assigned to this hashring to ``filename``.
         :param string description: If given, include a description next to the
-            index number of the ring from :attr:`FilteredBridgeHolder.rings`
+            index number of the ring from :attr:`FilteredBridgeSplitter.rings`
             the following bridges were assigned to. For example, if the
             description is ``"IPv6 obfs2 bridges"`` the line would read:
             ``"IPv6 obfs2 bridges ring=3"``.
@@ -374,7 +361,8 @@ class FixedBridgeSplitter(BridgeHolder):
         for index, ring in zip(xrange(len(self.rings)), self.rings):
             ring.dumpAssignments(filename, "%s ring=%s" % (description, index))
-class UnallocatedHolder(BridgeHolder):
+class UnallocatedHolder(object):
     """A pseudo-bridgeholder that ignores its bridges and leaves them
@@ -407,10 +395,11 @@ class UnallocatedHolder(BridgeHolder):
                 f.write("%s %s\n" % (bridge.hex_key, " ".join(desc).strip()))
-class BridgeSplitter(BridgeHolder):
-    """A BridgeHolder that splits incoming bridges up based on an hmac,
-       and assigns them to sub-bridgeholders with different probabilities.
-       Bridge-to-bridgeholder associations are recorded in a store.
+class BridgeSplitter(object):
+    """Splits incoming bridges up based on an HMAC, and assigns them to
+    sub-bridgeholders with different probabilities.  Bridge ←→ BridgeSplitter
+    associations are recorded in a store.
     def __init__(self, key):
         self.hmac = getHMACFunc(key, hex=True)
@@ -428,12 +417,13 @@ class BridgeSplitter(BridgeHolder):
         return n
     def addRing(self, ring, ringname, p=1):
-        """Add a new bridgeholder.
-           ring -- the bridgeholder to add.
-           ringname -- a string representing the bridgeholder.  This is used
-               to record which bridges have been assigned where in the store.
-           p -- the relative proportion of bridges to assign to this
-               bridgeholder.
+        """Add a new subring.
+        :param ring: The subring to add.
+        :param str ringname: This is used to record which bridges have been
+            assigned where in the store.
+        :param int p: The relative proportion of bridges to assign to this
+            bridgeholder.
         self.ringsByName[ringname] = ring
@@ -493,8 +483,8 @@ class BridgeSplitter(BridgeHolder):
             ring.dumpAssignments(f, "%s %s" % (description, name))
-class FilteredBridgeSplitter(BridgeHolder):
-    """A configurable BridgeHolder that filters bridges into subrings.
+class FilteredBridgeSplitter(object):
+    """Places bridges into subrings based upon sets of filters.
     The set of subrings and conditions used to assign :class:`Bridge`s should
     be passed to :meth:`~FilteredBridgeSplitter.addRing`.
@@ -513,7 +503,9 @@ class FilteredBridgeSplitter(BridgeHolder):
                  - ``ringname`` is a unique string identifying the subring.
                  - ``filterFn`` is a callable which filters Bridges in some
                    manner, i.e. by whether they are IPv4 or IPv6, etc.
-                 - ``subring`` is a :class:`BridgeHolder`.
+                 - ``subring`` is any of the horribly-implemented,
+                   I-guess-it-passes-for-some-sort-of-hashring classes in this
+                   module.
         :ivar hmac: DOCDOC
         :ivar bridges: DOCDOC
         :type distributorName: str
@@ -591,7 +583,6 @@ class FilteredBridgeSplitter(BridgeHolder):
     def addRing(self, subring, ringname, filterFn, populate_from=None):
         """Add a subring to this hashring.
-        :type subring: :class:`BridgeHolder`
         :param subring: The subring to add.
         :param str ringname: A unique name for identifying the new subring.
         :param filterFn: A function whose input is a :class:`Bridge`, and
@@ -617,10 +608,6 @@ class FilteredBridgeSplitter(BridgeHolder):
         # I suppose since it contains memory addresses, it *is* technically
         # likely to be a unique string, but it is messy.
-        if not isinstance(subring, BridgeHolder):
-            logging.fatal("%s hashring can't add invalid subring: %r"
-                          % (self.distributorName, subring))
-            return False
         if ringname in self.filterRings.keys():
             logging.fatal("%s hashring already has a subring named '%s'!"
                           % (self.distributorName, ringname))
diff --git a/lib/bridgedb/Main.py b/lib/bridgedb/Main.py
index 0bb319d..f382cc5 100644
--- a/lib/bridgedb/Main.py
+++ b/lib/bridgedb/Main.py
@@ -47,7 +47,7 @@ def load(state, hashring, clear=False):
     store them into our ``state.hashring`` instance. The ``state`` will be
     saved again at the end of this function.
-    :type hashring: :class:`BridgeSplitter <bridgedb.Bridges.BridgeHolder>`
+    :type hashring: :class:`~bridgedb.Bridges.BridgeSplitter`
     :param hashring: A class which provides a mechanism for HMACing
         Bridges in order to assign them to hashrings.
     :param boolean clear: If True, clear all previous bridges from the
@@ -323,7 +323,7 @@ def run(options, reactor=reactor):
         :ivar cfg: The current configuration, including any in-memory
             settings (i.e. settings whose values were not obtained from the
             config file, but were set via a function somewhere)
-        :type hashring: A :class:`bridgedb.Bridges.BridgeHolder`
+        :type hashring: A :class:`~bridgedb.Bridges.BridgeSplitter`
         :ivar hashring: A class which takes an HMAC key and splits bridges
             into their hashring assignments.
         :type proxyList: :class:`~bridgedb.proxy.ProxySet`
diff --git a/lib/bridgedb/persistent.py b/lib/bridgedb/persistent.py
index 1b2507c..c996daa 100644
--- a/lib/bridgedb/persistent.py
+++ b/lib/bridgedb/persistent.py
@@ -88,7 +88,8 @@ class State(jelly.Jellyable):
         modules which are known to be unjelliable/unpicklable so far are:
           - bridgedb.Dist
-          - bridgedb.Bridges.BridgeHolder and all other ``splitter`` classes
+          - bridgedb.Bridges, and all "splitter" and "ring" classes contained
+            within
         :property statefile: The filename to retrieve a pickled, jellied
             :class:`~bridgedb.persistent.State` instance from. (default:
diff --git a/lib/bridgedb/test/test_Main.py b/lib/bridgedb/test/test_Main.py
index 26012f7..e53256f 100644
--- a/lib/bridgedb/test/test_Main.py
+++ b/lib/bridgedb/test/test_Main.py
@@ -58,7 +58,7 @@ def mockUpdateBridgeHistory(bridges, timestamps):
                   (fingerprint, timestamp))
-class MockBridgeHolder(object):
+class MockHashring(object):
     def __init__(self):
         self._bridges = {}
     def __len__(self):
@@ -170,8 +170,8 @@ class MainTests(unittest.TestCase):
         self.state = Main.persistent.State(**self.config.__dict__)
         self.key = base64.b64decode('TvPS1y36BFguBmSOvhChgtXB2Lt+BOw0mGfz9SZe12Y=')
-        # Create a BridgeSplitter
-        self.hashring = MockBridgeHolder()
+        # Create a pseudo hashring
+        self.hashring = MockHashring()
         # Functions which some tests mock, which we'll need to re-replace
         # later in tearDown():

More information about the tor-commits mailing list