Drop force_no_field in template.remove(); implement test_remove().

- Also add tests for spacing in param names.

mwparserfromhell/nodes/template.py

@@ -142,9 +142,9 @@ class Template(Node):
                 return False
         return True
 
-    def _remove_without_field(self, param, i, force_no_field):
+    def _remove_without_field(self, param, i):
         """Return False if a parameter name should be kept, otherwise True."""
-        if not param.showkey and not force_no_field:
+        if not param.showkey:
             dependents = [not after.showkey for after in self.params[i+1:]]
             if any(dependents):
                 return False
@@ -266,22 +266,23 @@ class Template(Node):
         self.params.append(param)
         return param
 
-    def remove(self, name, keep_field=False, force_no_field=False):
+    def remove(self, name, keep_field=False):
         """Remove a parameter from the template whose name is *name*.
 
         If *keep_field* is ``True``, we will keep the parameter's name, but
         blank its value. Otherwise, we will remove the parameter completely
         *unless* other parameters are dependent on it (e.g. removing ``bar``
         from ``{{foo|bar|baz}}`` is unsafe because ``{{foo|baz}}`` is not what
-        we expected, so ``{{foo||baz}}`` will be produced instead), unless
-        *force_no_field* is also ``True``. If the parameter shows up multiple
-        times in the template, we will remove all instances of it (and keep
-        one if *keep_field* is ``True`` - that being the first instance if
-        none of the instances have dependents, otherwise that instance will be
-        kept).
+        we expected, so ``{{foo||baz}}`` will be produced instead).
+
+        If the parameter shows up multiple times in the template, we will
+        remove all instances of it (and keep one if *keep_field* is ``True`` -
+        the first instance if none have dependents, otherwise the one with
+        dependents will be kept).
         """
         name = name.strip() if isinstance(name, basestring) else str(name)
         removed = False
+        to_remove =[]
         for i, param in enumerate(self.params):
             if param.name.strip() == name:
                 if keep_field:
@@ -289,13 +290,15 @@ class Template(Node):
                         self._blank_param_value(param.value)
                         keep_field = False
                     else:
-                        self.params.remove(param)
+                        to_remove.append(param)
                 else:
-                    if self._remove_without_field(param, i, force_no_field):
-                        self.params.remove(param)
+                    if self._remove_without_field(param, i):
+                        to_remove.append(param)
                     else:
                         self._blank_param_value(param.value)
                 if not removed:
                     removed = True
         if not removed:
             raise ValueError(name)
+        for param in to_remove:
+            self.params.remove(param)

tests/test_template.py

@@ -98,7 +98,7 @@ class TestTemplate(TreeEqualityTestCase):
         """test Template.has_param()"""
         node1 = Template(wrap([Text("foobar")]))
         node2 = Template(wrap([Text("foo")]),
-                         [pgenh("1", "bar"), pgens("abc", "def")])
+                         [pgenh("1", "bar"), pgens("\nabc ", "def")])
         node3 = Template(wrap([Text("foo")]),
                          [pgenh("1", "a"), pgens("b", "c"), pgens("1", "d")])
         node4 = Template(wrap([Text("foo")]),
@@ -108,7 +108,7 @@ class TestTemplate(TreeEqualityTestCase):
         self.assertTrue(node2.has_param("abc"))
         self.assertFalse(node2.has_param("def"))
         self.assertTrue(node3.has_param("1"))
-        self.assertTrue(node3.has_param("b"))
+        self.assertTrue(node3.has_param(" b "))
         self.assertFalse(node4.has_param("b"))
         self.assertTrue(node3.has_param("b", False))
         self.assertTrue(node4.has_param("b", False))
@@ -123,7 +123,7 @@ class TestTemplate(TreeEqualityTestCase):
         node3p2 = pgens("1", "d")
         node3 = Template(wrap([Text("foo")]),
                          [pgenh("1", "a"), node3p1, node3p2])
-        node4p1 = pgens("b", " ")
+        node4p1 = pgens(" b", " ")
         node4 = Template(wrap([Text("foo")]), [pgenh("1", "a"), node4p1])
         self.assertRaises(ValueError, node1.get, "foobar")
         self.assertIs(node2p1, node2.get(1))
@@ -131,10 +131,56 @@ class TestTemplate(TreeEqualityTestCase):
         self.assertRaises(ValueError, node2.get, "def")
         self.assertIs(node3p1, node3.get("b"))
         self.assertIs(node3p2, node3.get("1"))
-        self.assertIs(node4p1, node4.get("b"))
+        self.assertIs(node4p1, node4.get("b "))
 
     # add
-    # remove
+
+    def test_remove(self):
+        """test Template.remove()"""
+        node1 = Template(wrap([Text("foobar")]))
+        node2 = Template(wrap([Text("foo")]), [pgenh("1", "bar"),
+                                               pgens("abc", "def")])
+        node3 = Template(wrap([Text("foo")]), [pgenh("1", "bar"),
+                                               pgens("abc", "def")])
+        node4 = Template(wrap([Text("foo")]), [pgenh("1", "bar"),
+                                               pgenh("2", "baz")])
+        node5 = Template(wrap([Text("foo")]), [
+            pgens(" a", "b"), pgens("b", "c"), pgens("a ", "d")])
+        node6 = Template(wrap([Text("foo")]), [
+            pgens(" a", "b"), pgens("b", "c"), pgens("a ", "d")])
+        node7 = Template(wrap([Text("foo")]), [
+            pgens("1  ", "a"), pgens("  1", "b"), pgens("2", "c")])
+        node8 = Template(wrap([Text("foo")]), [
+            pgens("1  ", "a"), pgens("  1", "b"), pgens("2", "c")])
+        node9 = Template(wrap([Text("foo")]), [
+            pgens("1  ", "a"), pgenh("1", "b"), pgenh("2", "c")])
+        node10 = Template(wrap([Text("foo")]), [
+            pgens("1  ", "a"), pgenh("1", "b"), pgenh("2", "c")])
+
+        node2.remove("1")
+        node2.remove("abc")
+        node3.remove(1, keep_field=True)
+        node3.remove("abc", keep_field=True)
+        node4.remove("1", keep_field=False)
+        node5.remove("a", keep_field=False)
+        node6.remove("a", keep_field=True)
+        node7.remove(1, keep_field=True)
+        node8.remove(1, keep_field=False)
+        node9.remove(1, keep_field=True)
+        node10.remove(1, keep_field=False)
+
+        self.assertRaises(ValueError, node1.remove, 1)
+        self.assertRaises(ValueError, node1.remove, "a")
+        self.assertRaises(ValueError, node2.remove, "1")
+        self.assertEquals("{{foo}}", node2)
+        self.assertEquals("{{foo||abc=}}", node3)
+        self.assertEquals("{{foo||baz}}", node4)
+        self.assertEquals("{{foo|b=c}}", node5)
+        self.assertEquals("{{foo| a=|b=c}}", node6)
+        self.assertEquals("{{foo|1  =|2=c}}", node7)
+        self.assertEquals("{{foo|2=c}}", node8)
+        self.assertEquals("{{foo||c}}", node9)
+        self.assertEquals("{{foo||c}}", node10)
 
 if __name__ == "__main__":
     unittest.main(verbosity=2)