Merge lp:~allenap/maas/urlencode-is-duckist into lp:~maas-committers/maas/trunk

[Gavin Panella]

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.

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.

[Julian Edwards (julian-edwards)]

Looks ok.

34 + # The tuple is important because this is used as input to urlencode,
35 + # which refuses to let ducks quack.

I'd expand this to include this explanation you've got in the commit msg about urlencode's requirement for a tuple. It just reads as a rant otherwise :)

review: Approve

[Gavin Panella (allenap)]

> I'd expand this to include this explanation you've got in the commit msg about
> urlencode's requirement for a tuple. It just reads as a rant otherwise :)

Okay, but it *is* a rant. It's late/early here :)

Thanks for the review.

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-09-26 13:07:08 +0000
+++ src/maascli/api.py	2012-09-27 03:05:23 +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 03:05:23 +0000
@@ -28,6 +28,8 @@
 from testtools.matchers import (
     Equals,
     Is,
+    IsInstance,
+    MatchesAll,
     MatchesListwise,
     )
 
@@ -127,6 +129,31 @@
             "%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
+        # urllib.urlencode, which 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.
+        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`."""