← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~allenap/maas/apiclient-urlencode-from-maascli into lp:maas

 

Gavin Panella has proposed merging lp:~allenap/maas/apiclient-urlencode-from-maascli into lp:maas with lp:~allenap/maas/apiclient-httplib2 as a prerequisite.

Commit message:
Move the non-insane urlencode from maascli into apiclient, and use it.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/apiclient-urlencode-from-maascli/+merge/128586
-- 
https://code.launchpad.net/~allenap/maas/apiclient-urlencode-from-maascli/+merge/128586
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/apiclient-urlencode-from-maascli into lp:maas.
=== modified file 'src/apiclient/maas_client.py'
--- src/apiclient/maas_client.py	2012-10-08 21:00:31 +0000
+++ src/apiclient/maas_client.py	2012-10-08 21:00:31 +0000
@@ -16,9 +16,10 @@
     'MAASOAuth',
     ]
 
-from urllib import urlencode
+from collections import Mapping
 
 from apiclient.multipart import encode_multipart_data
+from apiclient.utils import urlencode
 import httplib2
 import oauth.oauth as oauth
 
@@ -138,7 +139,7 @@
         """
         url = self._make_url(path)
         if params is not None and len(params) > 0:
-            url += "?" + urlencode(params)
+            url += "?" + urlencode(params.items())
         headers = {}
         self.auth.sign_request(url, headers)
         return url, headers
@@ -157,7 +158,7 @@
         if 'op' in params:
             params = dict(params)
             op = params.pop('op')
-            url += '?' + urlencode({'op': op})
+            url += '?' + urlencode([('op', op)])
         body, headers = encode_multipart_data(params, {})
         self.auth.sign_request(url, headers)
         return url, headers, body

=== modified file 'src/apiclient/tests/test_utils.py'
--- src/apiclient/tests/test_utils.py	2012-09-12 19:56:23 +0000
+++ src/apiclient/tests/test_utils.py	2012-10-08 21:00:31 +0000
@@ -12,8 +12,19 @@
 __metaclass__ = type
 __all__ = []
 
-from apiclient.utils import ascii_url
+from urllib import unquote
+
+from apiclient.utils import (
+    ascii_url,
+    urlencode,
+    )
+from django.utils.encoding import smart_unicode
 from maastesting.testcase import TestCase
+from testtools.matchers import (
+    Equals,
+    IsInstance,
+    MatchesAll,
+    )
 
 
 class TestHelpers(TestCase):
@@ -27,3 +38,23 @@
         self.assertEqual(
             b'http://example.com/', ascii_url('http://example.com/'))
         self.assertIsInstance(ascii_url('http://example.com'), bytes)
+
+    def test_urlencode_encodes_utf8_and_quotes(self):
+        # urlencode UTF-8 encodes unicode strings and applies standard query
+        # string quoting rules, and always returns a byte string.
+        data = [("=\u1234", "&\u4321")]
+        query = urlencode(data)
+        self.assertThat(
+            query, MatchesAll(
+                Equals(b"%3D%E1%88%B4=%26%E4%8C%A1"),
+                IsInstance(bytes)))
+
+    def test_urlencode_roundtrip_through_django(self):
+        # Check that urlencode's approach works with Django, as described on
+        # https://docs.djangoproject.com/en/dev/ref/unicode/.
+        data = [("=\u1234", "&\u4321")]
+        query = urlencode(data)
+        name, value = query.split(b"=")
+        name, value = unquote(name), unquote(value)
+        name, value = smart_unicode(name), smart_unicode(value)
+        self.assertEqual(data, [(name, value)])

=== modified file 'src/apiclient/utils.py'
--- src/apiclient/utils.py	2012-09-12 19:56:23 +0000
+++ src/apiclient/utils.py	2012-10-08 21:00:31 +0000
@@ -12,9 +12,11 @@
 __metaclass__ = type
 __all__ = [
     "ascii_url",
+    "urlencode",
     ]
 
 
+from urllib import quote_plus
 from urlparse import urlparse
 
 
@@ -26,3 +28,19 @@
             netloc=urlparts.netloc.encode("idna"))
         url = urlparts.geturl()
     return url.encode("ascii")
+
+
+def urlencode(data):
+    """A version of `urllib.urlencode` that isn't insane.
+
+    This only cares that `data` is an iterable of iterables. Each sub-iterable
+    must be of overall length 2, i.e. a name/value pair.
+
+    Unicode strings will be encoded to UTF-8. This is what Django expects; see
+    `smart_text` in the Django documentation.
+    """
+    enc = lambda string: quote_plus(
+        string.encode("utf-8") if isinstance(string, unicode) else string)
+    return b"&".join(
+        b"%s=%s" % (enc(name), enc(value))
+        for name, value in data)

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-10-08 11:18:45 +0000
+++ src/maascli/api.py	2012-10-08 21:00:31 +0000
@@ -28,7 +28,10 @@
 
 from apiclient.maas_client import MAASOAuth
 from apiclient.multipart import encode_multipart_data
-from apiclient.utils import ascii_url
+from apiclient.utils import (
+    ascii_url,
+    urlencode,
+    )
 import httplib2
 from maascli.command import (
     Command,
@@ -39,7 +42,6 @@
     handler_command_name,
     parse_docstring,
     safe_name,
-    urlencode,
     )
 
 

=== modified file 'src/maascli/tests/test_utils.py'
--- src/maascli/tests/test_utils.py	2012-10-08 15:10:58 +0000
+++ src/maascli/tests/test_utils.py	2012-10-08 21:00:31 +0000
@@ -12,16 +12,11 @@
 __metaclass__ = type
 __all__ = []
 
-from urllib import unquote
-
-from django.utils.encoding import smart_unicode
 from maascli import utils
 from maastesting.testcase import TestCase
 from testtools.matchers import (
     AfterPreprocessing,
     Equals,
-    IsInstance,
-    MatchesAll,
     MatchesListwise,
     )
 
@@ -177,26 +172,6 @@
         self.assertIsInstance(utils.ensure_trailing_slash(u"fred"), unicode)
         self.assertIsInstance(utils.ensure_trailing_slash(b"fred"), bytes)
 
-    def test_urlencode_encodes_utf8_and_quotes(self):
-        # urlencode UTF-8 encodes unicode strings and applies standard query
-        # string quoting rules, and always returns a byte string.
-        data = [("=\u1234", "&\u4321")]
-        query = utils.urlencode(data)
-        self.assertThat(
-            query, MatchesAll(
-                Equals(b"%3D%E1%88%B4=%26%E4%8C%A1"),
-                IsInstance(bytes)))
-
-    def test_urlencode_roundtrip_through_django(self):
-        # Check that urlencode's approach works with Django, as described on
-        # https://docs.djangoproject.com/en/dev/ref/unicode/.
-        data = [("=\u1234", "&\u4321")]
-        query = utils.urlencode(data)
-        name, value = query.split(b"=")
-        name, value = unquote(name), unquote(value)
-        name, value = smart_unicode(name), smart_unicode(value)
-        self.assertEqual(data, [(name, value)])
-
     def test_api_url(self):
         transformations = {
             "http://example.com/":

=== modified file 'src/maascli/utils.py'
--- src/maascli/utils.py	2012-10-08 13:34:40 +0000
+++ src/maascli/utils.py	2012-10-08 21:00:31 +0000
@@ -15,7 +15,6 @@
     "handler_command_name",
     "parse_docstring",
     "safe_name",
-    "urlencode",
     ]
 
 from functools import partial
@@ -24,7 +23,6 @@
     getdoc,
     )
 import re
-from urllib import quote_plus
 from urlparse import urlparse
 
 
@@ -100,19 +98,3 @@
     if re.search("/api/[0-9.]+/?$", url.path) is None:
         url = url._replace(path=url.path + "api/1.0/")
     return url.geturl()
-
-
-def urlencode(data):
-    """A version of `urllib.urlencode` that isn't insane.
-
-    This only cares that `data` is an iterable of iterables. Each sub-iterable
-    must be of overall length 2, i.e. a name/value pair.
-
-    Unicode strings will be encoded to UTF-8. This is what Django expects; see
-    `smart_text` in the Django documentation.
-    """
-    enc = lambda string: quote_plus(
-        string.encode("utf-8") if isinstance(string, unicode) else string)
-    return b"&".join(
-        b"%s=%s" % (enc(name), enc(value))
-        for name, value in data)