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

Proposed by Colin Watson
Status: Merged
Merged at revision: 576
Proposed branch: lp:~cjwatson/storm/clarify-no-allow-none-default-none-exception
Merge into: lp:storm
Diff against target: 168 lines (+64/-16)
6 files modified
NEWS (+6/-0)
storm/cextensions.c (+9/-0)
storm/tests/mocker.py (+4/-0)
storm/tests/properties.py (+8/-0)
storm/tests/variables.py (+30/-13)
storm/variables.py (+7/-3)
To merge this branch: bzr merge lp:~cjwatson/storm/clarify-no-allow-none-default-none-exception
Reviewer Review Type Date Requested Status
Ioana Lasc Approve
Review via email: mp+416974@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Ioana Lasc (ilasc) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2021-05-25 09:19:59 +0000
3+++ NEWS 2022-03-16 17:03:57 +0000
4@@ -1,6 +1,12 @@
5 0.26
6 ====
7
8+Improvements
9+------------
10+
11+- Clarify exception when creating a property with both allow_none=False and
12+ default=None.
13+
14 Bug fixes
15 ---------
16
17
18=== modified file 'storm/cextensions.c'
19--- storm/cextensions.c 2019-09-30 22:58:36 +0000
20+++ storm/cextensions.c 2022-03-16 17:03:57 +0000
21@@ -581,6 +581,15 @@
22 /* self._allow_none = False */
23 Py_INCREF(Py_False);
24 REPLACE(self->_allow_none, Py_False);
25+ /* if value is None: */
26+ if (value == Py_None) {
27+ /* raise_none_error(column, default=True) */
28+ tmp = PyObject_CallFunctionObjArgs(raise_none_error, column,
29+ Py_True, NULL);
30+ /* tmp should always be NULL here. */
31+ Py_XDECREF(tmp);
32+ goto error;
33+ }
34 }
35
36 /* if value is not Undef: */
37
38=== modified file 'storm/tests/mocker.py'
39--- storm/tests/mocker.py 2019-11-21 16:06:52 +0000
40+++ storm/tests/mocker.py 2022-03-16 17:03:57 +0000
41@@ -310,6 +310,10 @@
42 failUnlessIdentical = failUnlessIs
43 failIfIdentical = failIfIs
44
45+ if sys.version_info < (3, 2):
46+ def assertRaisesRegex(self, *args, **kwargs):
47+ return self.assertRaisesRegexp(*args, **kwargs)
48+
49
50 # --------------------------------------------------------------------
51 # Mocker.
52
53=== modified file 'storm/tests/properties.py'
54--- storm/tests/properties.py 2020-02-10 15:30:23 +0000
55+++ storm/tests/properties.py 2022-03-16 17:03:57 +0000
56@@ -846,6 +846,14 @@
57 self.assertRaises(NoneError, variable.set, None)
58 self.assertEqual(variable.get(), value)
59
60+ # Test default=None and allow_none=False (incoherent).
61+ Class.prop = func(name="name", default=None, allow_none=False)
62+ column = Class.prop.__get__(None, Class)
63+ self.assertEqual(column.name, "name")
64+ self.assertEqual(column.table, Class)
65+
66+ self.assertRaises(NoneError, column.variable_factory)
67+
68 # Test default_factory.
69 Class.prop = func(name="name", default_factory=lambda:value)
70 column = Class.prop.__get__(None, Class)
71
72=== modified file 'storm/tests/variables.py'
73--- storm/tests/variables.py 2020-02-10 15:30:23 +0000
74+++ storm/tests/variables.py 2022-03-16 17:03:57 +0000
75@@ -136,27 +136,44 @@
76
77 def test_set_none_with_allow_none(self):
78 variable = CustomVariable(allow_none=False)
79- self.assertRaises(NoneError, variable.set, None)
80+ self.assertRaisesRegex(
81+ NoneError, r"^None isn't acceptable as a value$",
82+ variable.set, None)
83
84 def test_set_none_with_allow_none_and_column(self):
85 column = Column("column_name")
86 variable = CustomVariable(allow_none=False, column=column)
87- try:
88- variable.set(None)
89- except NoneError as e:
90- self.assertTrue("column_name" in str(e))
91- else:
92- self.fail("NoneError not raised")
93+ self.assertRaisesRegex(
94+ NoneError, r"^None isn't acceptable as a value for column_name$",
95+ variable.set, None)
96
97 def test_set_none_with_allow_none_and_column_with_table(self):
98 column = Column("column_name", SQLToken("table_name"))
99 variable = CustomVariable(allow_none=False, column=column)
100- try:
101- variable.set(None)
102- except NoneError as e:
103- self.assertTrue("table_name.column_name" in str(e))
104- else:
105- self.fail("NoneError not raised")
106+ self.assertRaisesRegex(
107+ NoneError,
108+ r"^None isn't acceptable as a value for table_name.column_name$",
109+ variable.set, None)
110+
111+ def test_set_default_none_with_allow_none(self):
112+ self.assertRaisesRegex(
113+ NoneError, r"^None isn't acceptable as a default value$",
114+ CustomVariable, allow_none=False, value=None)
115+
116+ def test_set_default_none_with_allow_none_and_column(self):
117+ column = Column("column_name")
118+ self.assertRaisesRegex(
119+ NoneError,
120+ r"^None isn't acceptable as a default value for column_name$",
121+ CustomVariable, allow_none=False, value=None, column=column)
122+
123+ def test_set_default_none_with_allow_none_and_column_with_table(self):
124+ column = Column("column_name", SQLToken("table_name"))
125+ self.assertRaisesRegex(
126+ NoneError,
127+ r"^None isn't acceptable as a default value for "
128+ r"table_name.column_name$",
129+ CustomVariable, allow_none=False, value=None, column=column)
130
131 def test_set_with_validator(self):
132 args = []
133
134=== modified file 'storm/variables.py'
135--- storm/variables.py 2020-05-26 10:37:54 +0000
136+++ storm/variables.py 2022-03-16 17:03:57 +0000
137@@ -69,9 +69,10 @@
138 __slots__ = ()
139
140
141-def raise_none_error(column):
142+def raise_none_error(column, default=False):
143+ description = "default value" if default else "value"
144 if not column:
145- raise NoneError("None isn't acceptable as a value")
146+ raise NoneError("None isn't acceptable as a %s" % description)
147 else:
148 from storm.expr import compile, CompileError
149 name = column.name
150@@ -81,7 +82,8 @@
151 name = "%s.%s" % (table, name)
152 except CompileError:
153 pass
154- raise NoneError("None isn't acceptable as a value for %s" % name)
155+ raise NoneError(
156+ "None isn't acceptable as a %s for %s" % (description, name))
157
158
159 VariableFactory = partial
160@@ -139,6 +141,8 @@
161 """
162 if not allow_none:
163 self._allow_none = False
164+ if value is None:
165+ raise_none_error(column, default=True)
166 if value is not Undef:
167 self.set(value, from_db)
168 elif value_factory is not Undef:

Subscribers

People subscribed via source and target branches

to status/vote changes: