Merge lp:~cjwatson/storm/clarify-no-allow-none-default-none-exception into lp:storm

[Colin Watson]

Clarify exception for a property with allow_none=False and default=None.

Creating a property with `allow_none=False, default=None` doesn't make sense, but it's sometimes easy to set things up that way by accident during refactoring. Storm immediately raises `NoneError` when creating the variable in that case. However, it's not obvious that the exception is due to the bad default, and I spent some time with a colleague today debugging a situation that turned out to be due to this.

Change the exception message from "None isn't acceptable as a value" to "None isn't acceptable as a default value" in this case, to clue future travellers into the fact that the problem relates to the default.

[Ioana Lasc (ilasc)]

review: Approve

=== modified file 'NEWS'
--- NEWS	2021-05-25 09:19:59 +0000
+++ NEWS	2022-03-16 17:03:57 +0000
@@ -1,6 +1,12 @@
 0.26
 ====
 
+Improvements
+------------
+
+- Clarify exception when creating a property with both allow_none=False and
+  default=None.
+
 Bug fixes
 ---------
 

=== modified file 'storm/cextensions.c'
--- storm/cextensions.c	2019-09-30 22:58:36 +0000
+++ storm/cextensions.c	2022-03-16 17:03:57 +0000
@@ -581,6 +581,15 @@
         /* self._allow_none = False */
         Py_INCREF(Py_False);
         REPLACE(self->_allow_none, Py_False);
+        /* if value is None: */
+        if (value == Py_None) {
+            /* raise_none_error(column, default=True) */
+            tmp = PyObject_CallFunctionObjArgs(raise_none_error, column,
+                                               Py_True, NULL);
+            /* tmp should always be NULL here. */
+            Py_XDECREF(tmp);
+            goto error;
+        }
     }
 
     /* if value is not Undef: */

=== modified file 'storm/tests/mocker.py'
--- storm/tests/mocker.py	2019-11-21 16:06:52 +0000
+++ storm/tests/mocker.py	2022-03-16 17:03:57 +0000
@@ -310,6 +310,10 @@
     failUnlessIdentical = failUnlessIs
     failIfIdentical = failIfIs
 
+    if sys.version_info < (3, 2):
+        def assertRaisesRegex(self, *args, **kwargs):
+            return self.assertRaisesRegexp(*args, **kwargs)
+
 
 # --------------------------------------------------------------------
 # Mocker.

=== modified file 'storm/tests/properties.py'
--- storm/tests/properties.py	2020-02-10 15:30:23 +0000
+++ storm/tests/properties.py	2022-03-16 17:03:57 +0000
@@ -846,6 +846,14 @@
             self.assertRaises(NoneError, variable.set, None)
             self.assertEqual(variable.get(), value)
 
+            # Test default=None and allow_none=False (incoherent).
+            Class.prop = func(name="name", default=None, allow_none=False)
+            column = Class.prop.__get__(None, Class)
+            self.assertEqual(column.name, "name")
+            self.assertEqual(column.table, Class)
+
+            self.assertRaises(NoneError, column.variable_factory)
+
             # Test default_factory.
             Class.prop = func(name="name", default_factory=lambda:value)
             column = Class.prop.__get__(None, Class)

=== modified file 'storm/tests/variables.py'
--- storm/tests/variables.py	2020-02-10 15:30:23 +0000
+++ storm/tests/variables.py	2022-03-16 17:03:57 +0000
@@ -136,27 +136,44 @@
 
     def test_set_none_with_allow_none(self):
         variable = CustomVariable(allow_none=False)
-        self.assertRaises(NoneError, variable.set, None)
+        self.assertRaisesRegex(
+            NoneError, r"^None isn't acceptable as a value$",
+            variable.set, None)
 
     def test_set_none_with_allow_none_and_column(self):
         column = Column("column_name")
         variable = CustomVariable(allow_none=False, column=column)
-        try:
-            variable.set(None)
-        except NoneError as e:
-            self.assertTrue("column_name" in str(e))
-        else:
-            self.fail("NoneError not raised")
+        self.assertRaisesRegex(
+            NoneError, r"^None isn't acceptable as a value for column_name$",
+            variable.set, None)
 
     def test_set_none_with_allow_none_and_column_with_table(self):
         column = Column("column_name", SQLToken("table_name"))
         variable = CustomVariable(allow_none=False, column=column)
-        try:
-            variable.set(None)
-        except NoneError as e:
-            self.assertTrue("table_name.column_name" in str(e))
-        else:
-            self.fail("NoneError not raised")
+        self.assertRaisesRegex(
+            NoneError,
+            r"^None isn't acceptable as a value for table_name.column_name$",
+            variable.set, None)
+
+    def test_set_default_none_with_allow_none(self):
+        self.assertRaisesRegex(
+            NoneError, r"^None isn't acceptable as a default value$",
+            CustomVariable, allow_none=False, value=None)
+
+    def test_set_default_none_with_allow_none_and_column(self):
+        column = Column("column_name")
+        self.assertRaisesRegex(
+            NoneError,
+            r"^None isn't acceptable as a default value for column_name$",
+            CustomVariable, allow_none=False, value=None, column=column)
+
+    def test_set_default_none_with_allow_none_and_column_with_table(self):
+        column = Column("column_name", SQLToken("table_name"))
+        self.assertRaisesRegex(
+            NoneError,
+            r"^None isn't acceptable as a default value for "
+            r"table_name.column_name$",
+            CustomVariable, allow_none=False, value=None, column=column)
 
     def test_set_with_validator(self):
         args = []

=== modified file 'storm/variables.py'
--- storm/variables.py	2020-05-26 10:37:54 +0000
+++ storm/variables.py	2022-03-16 17:03:57 +0000
@@ -69,9 +69,10 @@
     __slots__ = ()
 
 
-def raise_none_error(column):
+def raise_none_error(column, default=False):
+    description = "default value" if default else "value"
     if not column:
-        raise NoneError("None isn't acceptable as a value")
+        raise NoneError("None isn't acceptable as a %s" % description)
     else:
         from storm.expr import compile, CompileError
         name = column.name
@@ -81,7 +82,8 @@
                 name = "%s.%s" % (table, name)
             except CompileError:
                 pass
-        raise NoneError("None isn't acceptable as a value for %s" % name)
+        raise NoneError(
+            "None isn't acceptable as a %s for %s" % (description, name))
 
 
 VariableFactory = partial
@@ -139,6 +141,8 @@
         """
         if not allow_none:
             self._allow_none = False
+            if value is None:
+                raise_none_error(column, default=True)
         if value is not Undef:
             self.set(value, from_db)
         elif value_factory is not Undef: