launchpad-reviewers team mailing list archive

[Colin Watson <mp+412134@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:remove-enumcol into launchpad:master.

Commit message:
Remove now-unused EnumCol

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

All call sites now use the Storm-style `DBEnum` instead.

In the process of updating tests for this, I noticed that DBEnum only checks the type of the `enum` parameter when the property is used (thus constructing the associated variable), rather than when the property is constructed; and it also failed to actually do the type check in practice due to a Python 3 porting bug.  This seems inconvenient, so I moved the type check earlier and fixed it for Python 3.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:remove-enumcol into launchpad:master.

diff --git a/lib/lp/services/database/doc/enumcol.txt b/lib/lp/services/database/doc/enumcol.txt
index ac5ad80..4685182 100644
--- a/lib/lp/services/database/doc/enumcol.txt
+++ b/lib/lp/services/database/doc/enumcol.txt
@@ -1,9 +1,8 @@
-An example of EnumCol with EnumeratedTypes
-==========================================
+An example of DBEnum with EnumeratedTypes
+=========================================
 
-EnumCol is a type of column that is used in SQLBase classes where the
-database representation is an integer and the code uses an enumerated
-type.
+DBEnum is a type of column that is used in Storm classes where the database
+representation is an integer and the code uses an enumerated type.
 
 Firstly we need an example table.
 
@@ -20,10 +19,10 @@ Firstly we need an example table.
     >>> import transaction
     >>> transaction.commit()
 
-The enumerated type that is used with the EnumCol must be a
+The enumerated type that is used with the DBEnum must be a
 DBEnumeratedType, with items that are instances of DBItem.
 
-Attempting to use a normal enumerated type for an enumcol will
+Attempting to use a normal enumerated type for a DBEnum will
 result in an error.
 
     >>> from lazr.enum import EnumeratedType, Item, use_template
@@ -32,11 +31,15 @@ result in an error.
     ...     ONE = Item("One")
     ...     TWO = Item("Two")
 
+    >>> from storm.locals import Int
     >>> from lp.services.database.constants import DEFAULT
-    >>> from lp.services.database.enumcol import EnumCol
-    >>> from lp.services.database.sqlbase import SQLBase
-    >>> class BadFooTest(SQLBase):
-    ...     foo = EnumCol(enum=PlainFooType, default=DEFAULT)
+    >>> from lp.services.database.enumcol import DBEnum
+    >>> from lp.services.database.interfaces import IStore
+    >>> from lp.services.database.stormbase import StormBase
+    >>> class BadFooTest(StormBase):
+    ...     __storm_table__ = "footest"
+    ...     id = Int(primary=True)
+    ...     foo = DBEnum(enum=PlainFooType, default=DEFAULT)
     Traceback (most recent call last):
     ...
     TypeError: <EnumeratedType 'PlainFooType'> must be a
@@ -59,12 +62,15 @@ result in an error.
 
 The database implementation class then refers to the enumerated type.
 
-    >>> class FooTest(SQLBase):
-    ...     foo = EnumCol(enum=FooType, default=DEFAULT)
+    >>> class FooTest(StormBase):
+    ...     __storm_table__ = "footest"
+    ...     id = Int(primary=True)
+    ...     foo = DBEnum(enum=FooType, default=DEFAULT)
 
 Create a row in the table.
 
-    >>> t = FooTest()
+    >>> store = IStore(FooTest)
+    >>> t = store.add(FooTest())
 
 The value of the foo column has been set to the default defined in the
 database, because we didn't specify one in the constructor.
@@ -72,7 +78,7 @@ database, because we didn't specify one in the constructor.
     >>> t.foo == FooType.ONE
     True
 
-You cannot use integers or strings as EnumCol values:
+You cannot use integers or strings as DBEnum values:
 
     >>> t.foo = 2
     Traceback (most recent call last):
@@ -130,24 +136,27 @@ schemas into one table. This works if you tell the column about both enums:
 
 Redefine the table with awareness of BarType:
 
-    >>> class FooTest(SQLBase):
-    ...     foo = EnumCol(schema=[FooType, BarType], default=DEFAULT)
+    >>> class FooTest(StormBase):
+    ...     __storm_table__ = "footest"
+    ...     id = Int(primary=True)
+    ...     foo = DBEnum(enum=[FooType, BarType], default=DEFAULT)
 
 We can assign items from either schema to the table;
 
-    >>> t = FooTest()
+    >>> t = store.add(FooTest())
     >>> t.foo = FooType.ONE
+    >>> store.flush()
     >>> t_id = t.id
-    >>> b = FooTest()
+    >>> b = store.add(FooTest())
     >>> b.foo = BarType.THREE
+    >>> store.flush()
     >>> b_id = b.id
 
 And reading back from the database correctly finds things from the schemas in
 the order given.
 
-    >>> from storm.store import AutoReload
-    >>> b.foo = AutoReload
-    >>> t.foo = AutoReload
+    >>> store.autoreload(b)
+    >>> store.autoreload(t)
     >>> b.foo == BarType.THREE
     True
     >>> t.foo == FooType.ONE
diff --git a/lib/lp/services/database/enumcol.py b/lib/lp/services/database/enumcol.py
index cf4867c..3da2741 100644
--- a/lib/lp/services/database/enumcol.py
+++ b/lib/lp/services/database/enumcol.py
@@ -1,13 +1,10 @@
 # Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-import warnings
-
 from lazr.enum import (
     DBEnumeratedType,
     DBItem,
     )
-from storm import sqlobject
 from storm.properties import SimpleProperty
 from storm.variables import Variable
 from zope.security.proxy import isinstance as zope_isinstance
@@ -15,7 +12,6 @@ from zope.security.proxy import isinstance as zope_isinstance
 
 __all__ = [
     'DBEnum',
-    'EnumCol',
     ]
 
 
@@ -27,7 +23,8 @@ def check_enum_type(enum):
 
 def check_type(enum):
     if type(enum) in (list, tuple):
-        map(check_enum_type, enum)
+        for element in enum:
+            check_enum_type(element)
     else:
         check_enum_type(enum)
 
@@ -37,12 +34,8 @@ class DBEnumVariable(Variable):
     __slots__ = ("_enum",)
 
     def __init__(self, *args, **kwargs):
-        enum = kwargs.pop("enum")
-        if type(enum) not in (list, tuple):
-            enum = (enum,)
-        self._enum = enum
-        check_type(self._enum)
-        super(DBEnumVariable, self).__init__(*args, **kwargs)
+        self._enum = kwargs.pop("enum")
+        super().__init__(*args, **kwargs)
 
     def parse_set(self, value, from_db):
         if from_db:
@@ -71,21 +64,9 @@ class DBEnumVariable(Variable):
 class DBEnum(SimpleProperty):
     variable_class = DBEnumVariable
 
-
-class EnumCol(sqlobject.PropertyAdapter, DBEnum):
-    def __init__(self, **kw):
-        # We want to end up using only the Storm style, not a mix of
-        # SQLObject and Storm.
-        warnings.warn(
-            "The SQLObject property EnumCol is deprecated; use the Storm "
-            "property DBEnum instead.",
-            DeprecationWarning, stacklevel=2)
-        try:
-            enum = kw.pop('enum')
-        except KeyError:
-            enum = kw.pop('schema')
+    def __init__(self, *args, **kwargs):
+        enum = kwargs.pop("enum")
+        if type(enum) not in (list, tuple):
+            enum = (enum,)
         check_type(enum)
-        self._kwargs = {
-            'enum': enum,
-            }
-        super().__init__(**kw)
+        super().__init__(enum=enum, *args, **kwargs)