Using Integer type with SelectWidget renders wrong selected values #81

Closed
domenkozar opened this Issue Mar 28, 2012 · 9 comments

Comments

Projects
None yet
4 participants
@domenkozar
Member

domenkozar commented Mar 28, 2012

Test case: https://gist.github.com/2225789

You will see first schema does not have selected value in output html, while second example does.

@dnouri

This comment has been minimized.

Show comment Hide comment
@dnouri

dnouri Mar 28, 2012

Member

From SelectWidget doctstring:

values
    A sequence of two-tuples (both values must be **string** or
    **unicode** values) indicating allowable, displayed values,
    e.g. ``( ('true', 'True'), ('false', 'False') )``.  The first
    element in the tuple is the value that should be returned when
    the form is posted.  The second is the display value.

I guess it'd be helpful to add a type check in SelectWidget.__init__. I've been bitten by this myself.

Member

dnouri commented Mar 28, 2012

From SelectWidget doctstring:

values
    A sequence of two-tuples (both values must be **string** or
    **unicode** values) indicating allowable, displayed values,
    e.g. ``( ('true', 'True'), ('false', 'False') )``.  The first
    element in the tuple is the value that should be returned when
    the form is posted.  The second is the display value.

I guess it'd be helpful to add a type check in SelectWidget.__init__. I've been bitten by this myself.

@domenkozar

This comment has been minimized.

Show comment Hide comment
@domenkozar

domenkozar Mar 28, 2012

Member

What would be the reason not to convert it to unicode if parameter is not instance of basestring?

Member

domenkozar commented Mar 28, 2012

What would be the reason not to convert it to unicode if parameter is not instance of basestring?

@mcdonc

This comment has been minimized.

Show comment Hide comment
@mcdonc

mcdonc Mar 29, 2012

Owner

I wouldn't be against converting a subset of types to str automagically (int is the only one I can really think of right now), but I wouldn't want the thing to try to convert any-old-thing to str, because that logic gets really desperate after a while and would end up causing even more confusion.

Owner

mcdonc commented Mar 29, 2012

I wouldn't be against converting a subset of types to str automagically (int is the only one I can really think of right now), but I wouldn't want the thing to try to convert any-old-thing to str, because that logic gets really desperate after a while and would end up causing even more confusion.

@dnouri dnouri closed this in 494db2b Mar 30, 2012

@dnouri

This comment has been minimized.

Show comment Hide comment
@dnouri

dnouri Mar 30, 2012

Member

Added a test to deformdemo in Pylons/deformdemo@740ae1f

Member

dnouri commented Mar 30, 2012

Added a test to deformdemo in Pylons/deformdemo@740ae1f

@mcdonc

This comment has been minimized.

Show comment Hide comment
@mcdonc

mcdonc Mar 30, 2012

Owner

You rock.

Owner

mcdonc commented Mar 30, 2012

You rock.

@dnouri

This comment has been minimized.

Show comment Hide comment
@dnouri

dnouri Mar 30, 2012

Member

Argh, silly. My fix will break when first value is non-ascii unicode. Will fix tomorrow.

I will nevertheless accept the compliment. ;-)

Member

dnouri commented Mar 30, 2012

Argh, silly. My fix will break when first value is non-ascii unicode. Will fix tomorrow.

I will nevertheless accept the compliment. ;-)

@JDeuce

This comment has been minimized.

Show comment Hide comment
@JDeuce

JDeuce Apr 24, 2012

Note there was a similar problem with RadioChoiceWidget, that the above commit patches.

JDeuce commented Apr 24, 2012

Note there was a similar problem with RadioChoiceWidget, that the above commit patches.

@mcdonc

This comment has been minimized.

Show comment Hide comment
@mcdonc

mcdonc Dec 30, 2012

Owner

This turned out to be a bad idea, because for template code like this:

   <option tal:repeat="(value, description) values"
           tal:attributes="selected value in cstruct and 'selected';
                           class field.widget.css_class"
           value="${value}">${description}</option>

the 'value' above will be a string after normalization while the item in cstruct might be an integer. This is kind of a clusterfuck at this point because it was done so long ago, but I may need to revert it, because I can't think of a sane way to leave it the way it is.

Owner

mcdonc commented Dec 30, 2012

This turned out to be a bad idea, because for template code like this:

   <option tal:repeat="(value, description) values"
           tal:attributes="selected value in cstruct and 'selected';
                           class field.widget.css_class"
           value="${value}">${description}</option>

the 'value' above will be a string after normalization while the item in cstruct might be an integer. This is kind of a clusterfuck at this point because it was done so long ago, but I may need to revert it, because I can't think of a sane way to leave it the way it is.

@mcdonc

This comment has been minimized.

Show comment Hide comment
@mcdonc

mcdonc Dec 30, 2012

Owner

Nope, it's even more clusterfucked than just reverting. I'll keep it as-is for now and cope.

Owner

mcdonc commented Dec 30, 2012

Nope, it's even more clusterfucked than just reverting. I'll keep it as-is for now and cope.

julsbreakdown pushed a commit to julsbreakdown/rdvchiro that referenced this issue Oct 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment