launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/urlencode-is-duckist into lp:maas.

Commit message:
Always return a 2-tuple from name_value_pair.

urllib.urlencode has some fairly duckist code. It doesn't let the data it consumes walk and quack like a duck. It insists that the first item in a non-dict sequence is a tuple. Of any size. It does this in the name of avoiding *string* input.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/urlencode-is-duckist/+merge/126584

urllib.urlencode has some fairly duckist code. It doesn't let the data
it consumes walk and quack like a duck. It insists that the first item
in a non-dict sequence is a tuple. Of any size. It does this in the
name of avoiding *string* input.
-- 
https://code.launchpad.net/~allenap/maas/urlencode-is-duckist/+merge/126584
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/urlencode-is-duckist into lp:maas.

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-09-26 13:07:08 +0000
+++ src/maascli/api.py	2012-09-27 01:09:22 +0000
@@ -241,7 +241,7 @@
     def name_value_pair(string):
         parts = string.split("=", 1)
         if len(parts) == 2:
-            return parts
+            return tuple(parts)
         else:
             raise CommandError(
                 "%r is not a name=value pair" % string)

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2012-09-26 13:17:05 +0000
+++ src/maascli/tests/test_api.py	2012-09-27 01:09:22 +0000
@@ -28,6 +28,8 @@
 from testtools.matchers import (
     Equals,
     Is,
+    IsInstance,
+    MatchesAll,
     MatchesListwise,
     )
 
@@ -127,6 +129,28 @@
             "%s" % error)
 
 
+class TestAction(TestCase):
+    """Tests for :class:`maascli.api.Action`."""
+
+    def test_name_value_pair_returns_2_tuple(self):
+        # The tuple is important because this is used as input to urlencode,
+        # which refuses to let ducks quack.
+        result = api.Action.name_value_pair("foo=bar")
+        self.assertThat(
+            result, MatchesAll(
+                Equals(("foo", "bar")),
+                IsInstance(tuple)))
+
+    def test_name_value_pair_demands_two_parts(self):
+        self.assertRaises(
+            CommandError, api.Action.name_value_pair, "foo bar")
+
+    def test_name_value_pair_does_not_strip_whitespace(self):
+        self.assertEqual(
+            (" foo ", " bar "),
+            api.Action.name_value_pair(" foo = bar "))
+
+
 class TestActionReSTful(TestCase):
     """Tests for ReSTful operations in `maascli.api.Action`."""