Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix SelectField: do not coerce None into a string #288

Merged
merged 1 commit into from Jun 12, 2018

Conversation

georgschoelly
Copy link
Contributor

@georgschoelly georgschoelly commented Jul 21, 2016

No description provided.

@@ -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>'''
Copy link
Contributor Author

@georgschoelly georgschoelly Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ftm ftm Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would it not be appropriate?

Copy link
Contributor Author

@georgschoelly georgschoelly Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@collisdigital
Copy link

@collisdigital collisdigital commented Mar 7, 2017

We hit this exact issue where we had a radio field whose name and value was "None" e.g.

Please select:
- None
- Low
- Medium
- High

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)
Copy link
Contributor

@ftm ftm Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = None

Copy link
Contributor Author

@georgschoelly georgschoelly Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinion on this.

Copy link
Contributor

@ftm ftm Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I'll change it when I merge it since I have to do it manually

@ftm ftm merged commit 111cfed into wtforms:master Jun 12, 2018
2 checks passed
ftm added a commit that referenced this issue Jun 12, 2018
@georgschoelly georgschoelly deleted the no_coerce_none branch Jun 12, 2018
@alswl
Copy link

@alswl alswl commented May 8, 2019

Any plan to update pypi version? @ftm

@ftm
Copy link
Contributor

@ftm ftm commented May 8, 2019

@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.

azmeuk added a commit to azmeuk/wtforms that referenced this issue Apr 18, 2020
Glandos added a commit to Glandos/ihatemoney that referenced this issue Jun 9, 2021
Glandos added a commit to spiral-project/ihatemoney that referenced this issue Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants