Skip to content
This repository

Conv accept #115

Merged
merged 4 commits into from over 1 year ago

3 participants

Tim Perevezentsev ods
Owner
Harut commented
  • Removed hack with wrapping to_python method
  • Explicit call of to_python when error handling and required check is not needed
  • No more mess with accept and to_python methods, Converter.convert method is added: to_python just converts value and may raise ValidationError, convert catches validation errors, checks for required and applies filters

minuses:

  • we have to check everywhere in the code, whether to use convert or accept methods
ods

accept() method seems OK, but convert() is certainly a bad name.

Owner

Any ideas for the name?

ods

Why you didn't make this in main branch? It's not related to the rest changes.

Essentially, the content of try: block is a part of to_python flow, not accept. If we will move it there, the code becomes more clear. But I don't know how to implement it without any wrappings or other ugly staff.

Tim Perevezentsev
Owner
riffm commented

:+1:

ods
Owner
ods commented

Good:

  • we now can fix long standing problems
  • simple and obvious interface of to_python for most cases and more flexible interface of accept for rare occasions when we need to fill errors for several fields

Bad:

  • no symetry: we have 2 interface methods for one direction and 1 for opposite.
Owner
Harut commented

simple and obvious interface of to_python

The thing still unsettling me about these changes is that to make the interface really obvious and consistent, as I wrote before, we have to force the content of try: block to be a part of to_python. I.e. required check and call of filters and validators.

How to implement it?

  • Wrap to_python as it was before? Or do some stuff with metaclasses as equal? Looks like hack
  • Implement this checks in Converter.to_python and force call Converter.to_python(self, value) every time the method is redefined? It's annoying and fragile.

May be anyone knows straight way?

Owner
Harut commented

I mean, if we write for exmaple:

    conv = Char(striptags, limit(3,250))
    # we expect that:
    conv.to_python('<p>Text</p>') == 'Text'
    conv.to_python('a') # raises ValidationError by length

In current (in the branch) implementation we will get

    conv.to_python('<p>Text</p>') == '<p>Text</p>'
    conv.to_python('a') == 'a'

    conv.accept('<p>Text</p>') == 'Text'
    conv.accept('a') == None # form.errors = {'name': 'blabla the string is too short'}
Harut merged commit 73f4a9b into from
Harut closed this
Harut deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
53 iktomi/forms/convs.py
@@ -72,7 +72,6 @@ def __init__(self, *args, **kwargs):
72 72 self._init_kwargs = kwargs
73 73 self.__dict__.update(kwargs)
74 74 self.validators_and_filters = args
75   - self.to_python = self._check(self.to_python)
76 75
77 76 # It is defined as read-only property to avoid setting it to True where
78 77 # converter doesn't support it.
@@ -91,24 +90,22 @@ def env(self):
91 90 def _is_empty(self, value):
92 91 return value in ('', [], {}, None)
93 92
94   - def _check(self, method):
95   - def wrapper(value, **kwargs):
96   - field, form = self.field, self.field.form
97   - try:
98   - value = method(value, **kwargs)
99   - if self.required and self._is_empty(value):
100   - form.errors[self.field.input_name] = self.error_required
101   - return self._existing_value
102   - for v in self.validators_and_filters:
103   - value = v(self, value)
104   - except ValidationError, e:
  93 + def accept(self, value, silent=False):
  94 + try:
  95 + value = self.to_python(value)
  96 + if self.required and self._is_empty(value):
  97 + raise ValidationError(self.error_required)
  98 + for v in self.validators_and_filters:
  99 + value = v(self, value)
  100 + except ValidationError, e:
  101 + if not silent:
  102 + field, form = self.field, self.field.form
105 103 form.errors[field.input_name] = e.message
106   - #NOTE: by default value for field is in python_data,
107   - # but this is not true for FieldList where data
108   - # is dynamic, so we set value to None for absent value.
109   - value = self._existing_value
110   - return value
111   - return wrapper
  104 + #NOTE: by default value for field is in python_data,
  105 + # but this is not true for FieldList where data
  106 + # is dynamic, so we set value to None for absent value.
  107 + value = self._existing_value
  108 + return value
112 109
113 110 def to_python(self, value):
114 111 """ custom converters should override this """
@@ -134,7 +131,9 @@ def assert_(self, expression, msg):
134 131
135 132 @property
136 133 def _existing_value(self):
137   - return self.field.parent.python_data.get(self.field.name)
  134 + if self.field is not None:
  135 + return self.field.parent.python_data.get(self.field.name)
  136 + return [] if self.multiple else None
138 137
139 138
140 139 class validator(object):
@@ -307,21 +306,15 @@ def from_python(self, value):
307 306 return conv.from_python(value)
308 307
309 308 def _safe_to_python(self, value):
310   - conv = self.conv(field=self.field)
311   - try:
312   - value = conv.to_python(value)
313   - except ValidationError:
314   - return None
  309 + # XXX hack
  310 + value = self.conv.accept(value, silent=True)
315 311 if value not in dict(self.choices):
316 312 return None
317 313 return value
318 314
319 315 def to_python(self, value):
320 316 if value == '':
321   - #XXX: check for multiple?
322   - if self.multiple:
323   - return []
324   - return None
  317 + return [] if self.multiple else None
325 318 if self.multiple:
326 319 value = [item for item in map(self._safe_to_python, value or [])
327 320 if item is not None]
@@ -335,8 +328,8 @@ def __iter__(self):
335 328 yield conv.from_python(python_value), label
336 329
337 330 def get_label(self, value):
338   - conv = self.conv(field=self.field)
339   - return dict(self.choices).get(conv.to_python(value))
  331 + value = self.conv.accept(value, silent=True)
  332 + return dict(self.choices).get(value)
340 333
341 334
342 335 class BaseDatetime(Converter):
11 iktomi/forms/fields.py
@@ -91,9 +91,6 @@ def id(self):
91 91 # insure unique IDs.
92 92 return '%s-%s' % (self.form.id, self.input_name)
93 93
94   - def to_python(self, value):
95   - return self.conv.to_python(value)
96   -
97 94 def from_python(self, value):
98 95 return self.conv.from_python(value)
99 96
@@ -173,7 +170,7 @@ def accept(self):
173 170 value = self.raw_value
174 171 if not self._check_value_type(value):
175 172 value = [] if self.multiple else self._null_value
176   - return self.to_python(value)
  173 + return self.conv.accept(value)
177 174
178 175
179 176 class AggregateField(BaseField):
@@ -223,7 +220,7 @@ def get_field(self, name):
223 220 def get_initial(self):
224 221 result = dict((field.name, field.get_initial())
225 222 for field in self.fields)
226   - return self.to_python(result)
  223 + return self.conv.accept(result, silent=True)
227 224
228 225 def set_raw_value(self, raw_data, value):
229 226 # fills in raw_data multidict, resulting keys are field's absolute names
@@ -241,7 +238,7 @@ def accept(self):
241 238 # readonly field
242 239 field.set_raw_value(self.form.raw_data,
243 240 field.from_python(result[field.name]))
244   - return self.to_python(result)
  241 + return self.conv.accept(result)
245 242
246 243 def render(self):
247 244 return self.env.template.render(self.template, field=self)
@@ -314,7 +311,7 @@ def accept(self):
314 311 result[field.name] = old[field.name]
315 312 else:
316 313 result[field.name] = field.accept()
317   - return self.to_python(result)
  314 + return self.conv.accept(result)
318 315
319 316 def set_raw_value(self, raw_data, value):
320 317 indeces = []
56 tests/forms/convs.py
@@ -17,14 +17,14 @@ class ConverterTests(unittest.TestCase):
17 17 def test_accept(self):
18 18 'Accept method of converter'
19 19 conv = init_conv(convs.Converter)
20   - value = conv.to_python('value')
  20 + value = conv.accept('value')
21 21 self.assertEqual(value, 'value')
22 22 self.assertEqual(conv.field.form.errors, {})
23 23
24 24 def test_to_python(self):
25 25 'Converter to_python method'
26 26 conv = init_conv(convs.Converter)
27   - value = conv.to_python('value')
  27 + value = conv.accept('value')
28 28 self.assertEqual(value, 'value')
29 29 self.assertEqual(conv.field.form.errors, {})
30 30
@@ -39,20 +39,26 @@ def test_obsolete(self):
39 39 'Convertor accepting obsolete parameters'
40 40 self.assertRaises(DeprecationWarning, convs.Converter, null=True)
41 41
  42 + def test_filter(self):
  43 + 'Convertor with filters'
  44 + conv = convs.Converter(lambda conv, x: x+'-1', lambda conv, x: x+'-2')
  45 + value = conv.accept('value', silent=True)
  46 + self.assertEqual(value, 'value-1-2')
  47 +
42 48
43 49 class IntConverterTests(unittest.TestCase):
44 50
45 51 def test_accept_valid(self):
46 52 'Accept method of Int converter'
47 53 conv = init_conv(convs.Int)
48   - value = conv.to_python('12')
  54 + value = conv.accept('12')
49 55 self.assertEqual(value, 12)
50 56 self.assertEqual(conv.field.form.errors, {})
51 57
52 58 def test_accept_null_value(self):
53 59 'Accept method of Int converter for None value'
54 60 conv = init_conv(convs.Int(required=False))
55   - value = conv.to_python('')
  61 + value = conv.accept('')
56 62 self.assertEqual(value, None)
57 63 self.assertEqual(conv.field.form.errors, {})
58 64
@@ -72,7 +78,7 @@ def test_accept_valid(self):
72 78 (1, 'result')
73 79 ], conv=convs.Int()))
74 80
75   - value = conv.to_python('1')
  81 + value = conv.accept('1')
76 82 self.assertEqual(value, 1)
77 83 self.assertEqual(conv.field.form.errors, {})
78 84
@@ -82,7 +88,7 @@ def test_accept_null_value(self):
82 88 (1, 'result')
83 89 ], conv=convs.Int(), required=False))
84 90
85   - value = conv.to_python('')
  91 + value = conv.accept('')
86 92 self.assertEqual(value, None)
87 93 self.assertEqual(conv.field.form.errors, {})
88 94
@@ -92,9 +98,9 @@ def test_accept_invalid_value(self):
92 98 (1, 'result')
93 99 ], conv=convs.Int(), required=False))
94 100
95   - value = conv.to_python('unknown')
  101 + value = conv.accept('unknown')
96 102 self.assertEqual(value, None)
97   - self.assertEqual(conv.field.form.errors.keys(), [conv.field.name])
  103 + self.assertEqual(conv.field.form.errors.keys(), [])
98 104
99 105 def test_accept_missing_value(self):
100 106 'Accept method of EnumChoice converter for None value'
@@ -102,7 +108,7 @@ def test_accept_missing_value(self):
102 108 (1, 'result')
103 109 ], conv=convs.Int(), required=False))
104 110
105   - value = conv.to_python('2')
  111 + value = conv.accept('2')
106 112 self.assertEqual(value, None)
107 113 self.assertEqual(conv.field.form.errors, {})
108 114
@@ -112,7 +118,7 @@ def test_decline_null_value(self):
112 118 (1, 'result')
113 119 ], conv=convs.Int(), required=True))
114 120
115   - value = conv.to_python('')
  121 + value = conv.accept('')
116 122 self.assertEqual(value, None)
117 123 self.assertEqual(conv.field.form.errors.keys(),
118 124 [conv.field.name])
@@ -122,7 +128,7 @@ def test_decline_invalid_value(self):
122 128 conv = init_conv(convs.EnumChoice(choices=[
123 129 (1, 'result')
124 130 ], conv=convs.Int(), required=True))
125   - value = conv.to_python('invalid')
  131 + value = conv.accept('invalid')
126 132 self.assertEqual(value, None)
127 133 self.assertEqual(conv.field.form.errors.keys(),
128 134 [conv.field.name])
@@ -132,7 +138,7 @@ def test_decline_missing_value(self):
132 138 conv = init_conv(convs.EnumChoice(choices=[
133 139 (1, 'result')
134 140 ], conv=convs.Int(), required=True))
135   - value = conv.to_python('2')
  141 + value = conv.accept('2')
136 142 self.assertEqual(value, None)
137 143 self.assertEqual(conv.field.form.errors.keys(),
138 144 [conv.field.name])
@@ -153,21 +159,21 @@ class CharConverterTests(unittest.TestCase):
153 159 def test_accept_valid(self):
154 160 'Accept method of Char converter'
155 161 conv = init_conv(convs.Char)
156   - value = conv.to_python('12')
  162 + value = conv.accept('12')
157 163 self.assertEqual(value, u'12')
158 164 self.assertEqual(conv.field.form.errors, {})
159 165
160 166 def test_accept_null_value(self):
161 167 'Accept method of Char converter for None value'
162 168 conv = init_conv(convs.Char(required=False))
163   - value = conv.to_python('')
  169 + value = conv.accept('')
164 170 self.assertEqual(value, '')
165 171 self.assertEqual(conv.field.form.errors, {})
166 172
167 173 def test_accept_null_value_regex(self):
168 174 'Accept empty value by Char converter with non-empty regexp'
169 175 conv = init_conv(convs.Char(regex='.+', required=False))
170   - value = conv.to_python('')
  176 + value = conv.accept('')
171 177 self.assertEqual(value, '') # XXX
172 178 self.assertEqual(conv.field.form.errors, {})
173 179
@@ -175,9 +181,9 @@ def test_regex_error(self):
175 181 conv = init_conv(convs.Char(regex='ZZZ', required=True))
176 182 # XXX This should look like the following:
177 183 #with self.assertRaises(convs.ValidationError) as cm:
178   - # conv.to_python('AAA')
  184 + # conv.accept('AAA')
179 185 #self.assertIn(conv.regex, cm.exception.message)
180   - conv.to_python('AAA')
  186 + conv.accept('AAA')
181 187 field_name = conv.field.name
182 188 errors = conv.field.form.errors
183 189 self.assertEqual(conv.field.form.errors.keys(), [field_name])
@@ -193,19 +199,19 @@ def test_from_python(self):
193 199 def test_strip(self):
194 200 'convs.Char.strip tests'
195 201 conv = init_conv(convs.Char(regex="\d+"))
196   - value = conv.to_python(' 12')
  202 + value = conv.accept(' 12')
197 203 self.assertEqual(value, u'12')
198 204 self.assertEqual(conv.field.form.errors, {})
199 205
200 206 conv = init_conv(convs.Char(strip=False))
201   - value = conv.to_python(' 12')
  207 + value = conv.accept(' 12')
202 208 self.assertEqual(value, u' 12')
203 209 self.assertEqual(conv.field.form.errors, {})
204 210
205 211 def test_strip_required(self):
206 212 'convs.Char.strip tests for required'
207 213 conv = init_conv(convs.Char(required=True, strip=True))
208   - value = conv.to_python(' ')
  214 + value = conv.accept(' ')
209 215 self.assertEqual(value, None) # XXX
210 216 field_name = conv.field.name
211 217 self.assertEqual(conv.field.form.errors.keys(), [field_name])
@@ -215,19 +221,19 @@ class BoolConverterTests(unittest.TestCase):
215 221
216 222 def test_accept_true(self):
217 223 conv = init_conv(convs.Bool)
218   - value = conv.to_python('xx')
  224 + value = conv.accept('xx')
219 225 self.assertEqual(value, True)
220 226 self.assertEqual(conv.field.form.errors, {})
221 227
222 228 def test_accept_false(self):
223 229 conv = init_conv(convs.Bool)
224   - value = conv.to_python('')
  230 + value = conv.accept('')
225 231 self.assertEqual(value, False)
226 232 self.assertEqual(conv.field.form.errors, {})
227 233
228 234 def test_required(self):
229 235 conv = init_conv(convs.Bool(required=True))
230   - value = conv.to_python('')
  236 + value = conv.accept('')
231 237 self.assertEqual(value, False) # XXX is this right?
232 238 field_name = conv.field.name
233 239 self.assertEqual(conv.field.form.errors, {})
@@ -252,7 +258,7 @@ def test_accept_valid(self):
252 258 '''Date converter to_python method'''
253 259 from datetime import date
254 260 conv = init_conv(convs.Date(format="%d.%m.%Y"))
255   - self.assertEqual(conv.to_python('31.01.1999'), date(1999, 1, 31))
  261 + self.assertEqual(conv.accept('31.01.1999'), date(1999, 1, 31))
256 262 self.assertEqual(conv.field.form.errors, {})
257 263
258 264 def test_readable_format(self):
@@ -293,7 +299,7 @@ def test_to_python(self):
293 299 '''Time converter to_python method'''
294 300 from datetime import time
295 301 conv = init_conv(convs.Time)
296   - self.assertEqual(conv.to_python('12:30'), time(12, 30))
  302 + self.assertEqual(conv.accept('12:30'), time(12, 30))
297 303 self.assertEqual(conv.field.form.errors, {})
298 304
299 305

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.