fix SelectField: do not coerce None into a string #288
Conversation
| @@ -367,7 +367,7 @@ def test_text_coercion(self): | |||
| form.a(), | |||
| '''<ul id="a">''' | |||
| '''<li><input id="a-0" name="a" type="radio" value="True"> <label for="a-0">yes</label></li>''' | |||
| '''<li><input checked id="a-1" name="a" type="radio" value="False"> <label for="a-1">no</label></li></ul>''' | |||
| '''<li><input id="a-1" name="a" type="radio" value="False"> <label for="a-1">no</label></li></ul>''' | |||
There was a problem hiding this comment.
Instead of changing the test result, we could also pass default=False to the RadioField constructor.
However, I don't think it's appropriate that checked is set here, so I changed the test result.
There was a problem hiding this comment.
Why would it not be appropriate?
There was a problem hiding this comment.
There's no reason to select the second choice:
make_form(a=RadioField(choices=[(True, 'yes'), (False, 'no')], coerce=coerce_func))
Having none of the options pre-selected is more natural.
|
We hit this exact issue where we had a radio field whose name and value was WTForms couldn't distinguish between the user selecting the "None" option and not selecting an option at all and also meant that the None option was pre-selected in the radio field before the user had interacted with it. Pretty horribly bug! We've worked around it with a custom coerce: ONSdigital/eq-survey-runner#1015 But ideally this should be fixed in WTForms directly; what's the status on this one? |
| # protect against coercing None, | ||
| # such as in text_type(None) -> "None" | ||
| if value is None: | ||
| raise ValueError() | ||
| self.data = self.coerce(value) |
There was a problem hiding this comment.
I don't like how this is raising an error just to be caught, would it not be better to do
try:
self.data = None if value is None else self.coerce(value)
except (ValueError, TypeError):
self.data = NoneThere was a problem hiding this comment.
I have no strong opinion on this.
There was a problem hiding this comment.
In that case I'll change it when I merge it since I have to do it manually
|
Any plan to update pypi version? @ftm |
|
@alswl I do not control when new versions are released. Please stop posting these comments on random issues and PRs, if you want to discuss this then please open a new issue. |
No description provided.