choices: add widgets.StrChoiceMapper and make this the default
3 files changed, 118 insertions(+), 56 deletions(-)

M doc/fields.rst
M morf/tests/test_widgets.py
M morf/widgets.py
M doc/fields.rst +38 -29
@@ 131,22 131,29 @@ list of choices. Set ``validate_choices=
                                     validate_choices=False)
 
 
-Choice fields are keyed on the json encoded representation of the value.
-The ``value_mapper`` argument can override this.
+Mapping choice values to strings
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 When rendered in HTML forms choice values must be converted to strings, and
 then back again when the form is submitted and processed.
 
-The default strategy is to convert your choice keys to a JSON representation.
-This handles the common cases of strings and integer keys well (and also
-``None``).
+Starting from version 2.0, :class:`morf.fields.Choice` fields with non-string
+values will have their values converted to strings when rendered in HTML.
+
+Two other options exist.
 
-An alternative implementation is offered, which adds a key based on a simple indexing of the items based on their position in the list. For example, the choices::
+:class:`~morf.widgets.JSONChoiceMapper` converts choice values to JSON when rendering as HTML. This was the default behaviour prior to v2.0.
+
+:class:`~morf.widgets.IndexChoiceMapper` uses the position of each value in the choice list as the key. For example, the choices:
+
+..code-block::python
 
     [('11:00', 'Elevenses'), ('13:00', 'Lunch time'), ('16:00', 'Tea time')]
 
 Would result in options indexed as follows::
 
+..code-block::html
+
     <select>
         <option value="0">Elevenses</option>
         <option value="1">Lunch time</option>

          
@@ 157,34 164,36 @@ When a user selects for example value ``
 morf looks up the index and populates the field with the
 original object, in this case ``16:00``
 
-The advantage with this strategy is that object identity is preserved.
-You can use any object as a value
-(perhaps a `:class:datetime.time` object in this case)
-and your form field will be populated with the very same object,
-thus elimating all the tedious mucking about converting values to
-strings and back again.
-
-However while this sounds neat, in practice there are major disadvantages:
-
-- It's fragile in the case that the list of values changes.
+To enable this behaviour use the ``mapper`` argument when creating the
+widget:
 
-- It's hard to test — to create test values for a choice field
-  you have to know the index of the value you want.
-
-- It's hard to generate values for the field
-  from other parts of your application
-  (eg imagine a search form has a 'category' choice.
-  Now you can't easily generate search URLs with
-  ``category=...`` in the querystring)
-
-To enable this behaviour use the ``mapper`` argument when creating the
-widget::
+..code-block::python
 
     from morf import fields, widgets
 
+    # JSON-encoded choices
     choicefield = fields.Choice(
-                    choices=['a', 'b', 'c'],
-                    widget=widgets.Select(mapper=widgets.IndexChoiceMapper())
+        choices=['a', 'b', 'c'],
+        widget=widgets.Select(mapper=widgets.JSONChoiceMapper())
+    )
+
+    # Index choices
+    choicefield = fields.Choice(
+        choices=['a', 'b', 'c'],
+        widget=widgets.Select(mapper=widgets.IndexChoiceMapper())
+    )
+
+To revert to the pre-2.0 default of using the
+:class:`~morf.widgets.JSONChoiceMapper` globally, add this code to your
+application:
+
+..code-block::python
+
+    from morf import widgets
+
+    widgets.SingleChoiceWidget.choice_mapper = widgets.JSONChoiceMapper
+
+
 
 fields.MultipleChoice
 ~~~~~~~~~~~~~~~~~~~~~

          
M morf/tests/test_widgets.py +59 -26
@@ 221,10 221,10 @@ class TestSelect(WidgetTestCase):
     field = fields.Str(choices=[("Y", "yes"), ("N", "no")])
 
     def test_it_renders(self):
-        expect(self.rendered({"s": '"N"'})) == (
+        assert self.rendered({"s": "N"}) == (
             '<select name="s" required="">'
-            '<option value="&#34;Y&#34;">yes</option>'
-            '<option selected="selected" value="&#34;N&#34;">no</option>'
+            '<option value="Y">yes</option>'
+            '<option selected="selected" value="N">no</option>'
             "</select>"
         )
 

          
@@ 235,10 235,10 @@ class TestSelect(WidgetTestCase):
         widget = widgets.Select()
         field = copy(self.field)
         field.choices = ["a", "a"]
-        expect(self.rendered({"s": '"a"'}, widget=widget, field=field)) == (
+        expect(self.rendered({"s": "a"}, widget=widget, field=field)) == (
             '<select name="s" required="">'
-            '<option selected="selected" value="&#34;a&#34;">a</option>'
-            '<option value="&#34;a&#34;">a</option>'
+            '<option selected="selected" value="a">a</option>'
+            '<option value="a">a</option>'
             "</select>"
         )
 

          
@@ 281,7 281,7 @@ class TestSelect(WidgetTestCase):
         r = f.as_p().render()
         expect(r).contains('<optgroup label="upper"><option value="65">A')
         expect(r).contains(
-            '<optgroup label="lower">' '<option value="&#34;a&#34;">a'
+            '<optgroup label="lower">' '<option value="a">a'
         )
 
 

          
@@ 292,9 292,9 @@ class TestRadioGroup(WidgetTestCase):
 
     def test_it_renders(self):
         expect(self.rendered()) == (
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label> "
-            '<input name="s" required="" type="radio" value="&#34;N&#34;" /> '
+            '<input name="s" required="" type="radio" value="N" /> '
             "<label>no</label>"
         )
 

          
@@ 307,27 307,27 @@ class TestRadioGroup(WidgetTestCase):
     def test_it_adds_container(self):
         expect(self.rendered(widget=widgets.RadioGroup(class_="foo"))) == (
             '<div class="foo">'
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label> "
-            '<input name="s" required="" type="radio" value="&#34;N&#34;" /> '
+            '<input name="s" required="" type="radio" value="N" /> '
             "<label>no</label>"
             "</div>"
         )
 
     def test_it_renders_text_first(self):
         expect(self.rendered(widget=widgets.RadioGroup.label_first())) == (
-            '<label>yes <input name="s" required="" type="radio" value="&#34;Y&#34;" />'
+            '<label>yes <input name="s" required="" type="radio" value="Y" />'
             "</label> "
-            '<label>no <input name="s" required="" type="radio" value="&#34;N&#34;" />'
+            '<label>no <input name="s" required="" type="radio" value="N" />'
             "</label>"
         )
 
     def test_it_renders_as_ul(self):
         expect(self.rendered(widget=widgets.RadioGroup.as_ul())) == (
             "<ul>"
-            '<li><input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<li><input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label></li>"
-            '<li><input name="s" required="" type="radio" value="&#34;N&#34;" /> '
+            '<li><input name="s" required="" type="radio" value="N" /> '
             "<label>no</label></li>"
             "</ul>"
         )

          
@@ 336,10 336,10 @@ class TestRadioGroup(WidgetTestCase):
         expect(self.rendered(widget=widgets.RadioGroup.as_table())) == (
             "<table><tbody>"
             "<tr><td>"
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /></td>'
+            '<input name="s" required="" type="radio" value="Y" /></td>'
             "<td><label>yes</label></td></tr>"
             "<tr><td>"
-            '<input name="s" required="" type="radio" value="&#34;N&#34;" /></td>'
+            '<input name="s" required="" type="radio" value="N" /></td>'
             "<td><label>no</label></td></tr>"
             "</tbody></table>"
         )

          
@@ 349,7 349,7 @@ class TestRadioGroup(WidgetTestCase):
         r = self.rendered(widget=widgets.RadioGroup(), field=field)
         expect(r) == (
             "<fieldset><legend>foo</legend>"
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label>"
             "</fieldset>"
         )

          
@@ 372,14 372,14 @@ class TestCheckboxGroup(WidgetTestCase):
 
     def test_it_renders(self):
         expect(self.rendered()) == (
-            '<input name="s" type="checkbox" value="&#34;Y&#34;" /> '
+            '<input name="s" type="checkbox" value="Y" /> '
             "<label>yes</label>"
         )
 
     def test_it_renders_as_ul(self):
         expect(self.rendered(widget=widgets.CheckboxGroup.as_ul())) == (
             "<ul>"
-            '<li><input name="s" type="checkbox" value="&#34;Y&#34;" /> '
+            '<li><input name="s" type="checkbox" value="Y" /> '
             "<label>yes</label></li>"
             "</ul>"
         )

          
@@ 388,7 388,7 @@ class TestCheckboxGroup(WidgetTestCase):
         expect(self.rendered(widget=widgets.CheckboxGroup.as_table())) == (
             "<table><tbody>"
             "<tr><td>"
-            '<input name="s" type="checkbox" value="&#34;Y&#34;" /></td>'
+            '<input name="s" type="checkbox" value="Y" /></td>'
             "<td><label>yes</label></td></tr>"
             "</tbody></table>"
         )

          
@@ 409,9 409,7 @@ class TestCheckboxGroup(WidgetTestCase):
             initial=["a", "c"],
         )
         checked = r.findall(".//input")
-        expect(
-            [c.attrib["value"] for c in checked if "checked" in c.attrib]
-        ) == ['"a"', '"c"']
+        assert [c.attrib["value"] for c in checked if "checked" in c.attrib] == ["a", "c"]
 
     def test_it_places_fieldname_in_inputs_not_container(self):
         el = self.rendered_dom(

          
@@ 421,10 419,10 @@ class TestCheckboxGroup(WidgetTestCase):
 
         # Has it assigned the fieldname as a input element atrribute?
         for inp in el.findall(".//input"):
-            expect(inp.attrib["name"]) == "foo"
+            assert inp.attrib["name"] == "foo"
 
         # Has it not assigned the fieldname as a container element atrribute?
-        expect(el.attrib) == {}
+        assert el.attrib == {}
 
 
 class TestJSONChoiceMapper(object):

          
@@ 467,6 465,41 @@ class TestJSONChoiceMapper(object):
             assert self.mapper.choice_map(self.choice_items)[key] is Undefined
 
 
+class TestStrChoiceMapper(object):
+
+    mapper = widgets.StrChoiceMapper()
+    choice_items = [
+        (1, "one"),
+        ("two", "two"),
+        (None, "nothing"),
+        ("optgroup_label", choices.OptGroup(["a", "b"])),
+    ]
+
+    def test_it_indexes_choices(self):
+        indexed_choices = self.mapper.indexed_choices(self.choice_items)
+        indexed_choices = listify(indexed_choices)
+        assert indexed_choices == [
+            ("1", 1, "one"),
+            ("two", "two", "two"),
+            ("None", None, "nothing"),
+            (
+                widgets.ChoiceMapper.OPTGROUP,
+                [("a", "a", "a"), ("b", "b", "b")],
+                "optgroup_label",
+            ),
+        ]
+
+    def test_it_maps_keys_to_values(self):
+        expected_mapped_values = {
+            "1": 1,
+            "two": "two",
+            "a": "a",
+            "None": None,
+        }
+        for key, expected in expected_mapped_values.items():
+            assert self.mapper.choice_map(self.choice_items)[key] == expected
+
+
 class TestIndexChoiceMapper(object):
 
     mapper = widgets.IndexChoiceMapper()

          
M morf/widgets.py +21 -1
@@ 310,6 310,26 @@ class JSONChoiceMapper(ChoiceMapper):
             return Undefined
 
 
+class StrChoiceMapper(ChoiceMapper):
+    def indexed_choices(self, choices):
+        for value, label in choices:
+            if isinstance(label, OptGroup):
+                yield (self.OPTGROUP, self.indexed_choices(label), str(value))
+            else:
+                yield str(value), value, label
+
+    def choice_map(self, choices):
+        def build_map(choices):
+            result = {}
+            for value, label in choices:
+                if isinstance(label, OptGroup):
+                    result.update(build_map(label))
+                else:
+                    result[str(value)] = value
+            return result
+        return build_map(choices)
+
+
 class IndexChoiceMapper(ChoiceMapper):
     """
     A :class:`ChoiceMapper` implementation that adds a integer index based

          
@@ 359,7 379,7 @@ class SingleChoiceWidget(Widget):
     """
 
     choices = None
-    choice_mapper = JSONChoiceMapper()
+    choice_mapper = StrChoiceMapper()
 
     def __init__(self, *args, **kwargs):
         self.choice_mapper = kwargs.pop("mapper", self.choice_mapper)