launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/headers-optional into lp:maas.

Commit message:
maas-cli only shows the response line and headers when the -d/--debug flag is used.

Previously these were always shown. Hiding them is important so that output from maas-cli can be usefully piped to other programs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/headers-optional/+merge/128075
-- 
https://code.launchpad.net/~allenap/maas/headers-optional/+merge/128075
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/headers-optional into lp:maas.

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-10-02 12:39:54 +0000
+++ src/maascli/api.py	2012-10-04 17:22:21 +0000
@@ -14,6 +14,7 @@
     "register",
     ]
 
+from email.message import Message
 from getpass import getpass
 import httplib
 from itertools import chain
@@ -87,6 +88,48 @@
     return json.loads(content)
 
 
+def get_response_content_type(response):
+    """Returns the response's content-type, without parameters.
+
+    If the content-type was not set in the response, returns `None`.
+
+    :type response: :class:`httplib2.Response`
+    """
+    try:
+        content_type = response["content-type"]
+    except KeyError:
+        return None
+    else:
+        message = Message()
+        message.set_type(content_type)
+        return message.get_content_type()
+
+
+def is_response_textual(response):
+    """Is the response body text?"""
+    content_type = get_response_content_type(response)
+    return (
+        content_type.endswith("/json") or
+        content_type.startswith("text/"))
+
+
+def print_headers(headers, file=sys.stdout):
+    """Show an HTTP response in a human-friendly way.
+
+    :type headers: :class:`httplib2.Response`, or :class:`dict`
+    """
+    # Function to change headers like "transfer-encoding" into
+    # "Transfer-Encoding".
+    cap = lambda header: "-".join(
+        part.capitalize() for part in header.split("-"))
+    # Format string to prettify reporting of response headers.
+    form = "%%%ds: %%s" % (
+        max(len(header) for header in headers) + 2)
+    # Print the response.
+    for header in sorted(headers):
+        print(form % (cap(header), headers[header]), file=file)
+
+
 class cmd_login(Command):
     """Log-in to a remote API, storing its description and credentials.
 
@@ -203,6 +246,9 @@
             parser.add_argument(param)
         parser.add_argument(
             "data", type=self.name_value_pair, nargs="*")
+        parser.add_argument(
+            "-d", "--debug", action="store_true", default=False,
+            help="Display more information about API responses.")
 
     def __call__(self, options):
         # TODO: this is el-cheapo URI Template
@@ -226,8 +272,11 @@
         response, content = http.request(
             uri, self.method, body=body, headers=headers)
 
-        # TODO: decide on how to display responses to users.
-        self.print_response(response, content)
+        # Output.
+        if options.debug:
+            self.print_debug(response)
+        self.print_response(
+            response, content, options.debug)
 
         # 2xx status codes are all okay.
         if response.status // 100 != 2:
@@ -276,29 +325,24 @@
         auth = MAASOAuth(*credentials)
         auth.sign_request(uri, headers)
 
+    @staticmethod
+    def print_debug(response):
+        """Dump the response line and headers to stderr."""
+        print(response.status, response.reason, file=sys.stderr)
+        print(file=sys.stderr)
+        print_headers(response, file=sys.stderr)
+        print(file=sys.stderr)
+
     @classmethod
     def print_response(cls, response, content):
-        """Show an HTTP response in a human-friendly way."""
-        # Print the response.
-        print(response.status, response.reason)
-        print()
-        cls.print_headers(response)
-        print()
-        print(content)
+        """Print the response body if it's textual.
 
-    @staticmethod
-    def print_headers(headers):
-        """Show an HTTP response in a human-friendly way."""
-        # Function to change headers like "transfer-encoding" into
-        # "Transfer-Encoding".
-        cap = lambda header: "-".join(
-            part.capitalize() for part in header.split("-"))
-        # Format string to prettify reporting of response headers.
-        form = "%%%ds: %%s" % (
-            max(len(header) for header in headers) + 2)
-        # Print the response.
-        for header in sorted(headers):
-            print(form % (cap(header), headers[header]))
+        Otherwise otherwise write it raw to stdout.
+        """
+        if is_response_textual(response):
+            print(content)  # Trailing newline, might encode.
+        else:
+            sys.stdout.write(content)  # Raw, binary.
 
 
 def register_actions(profile, handler, parser):

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2012-10-02 12:56:40 +0000
+++ src/maascli/tests/test_api.py	2012-10-04 17:22:21 +0000
@@ -127,6 +127,50 @@
             "Expected application/json, got: text/css",
             "%s" % error)
 
+    def test_get_response_content_type_returns_content_type_header(self):
+        response = httplib2.Response(
+            {"content-type": "application/json"})
+        self.assertEqual(
+            "application/json",
+            api.get_response_content_type(response))
+
+    def test_get_response_content_type_omits_parameters(self):
+        response = httplib2.Response(
+            {"content-type": "application/json; charset=utf-8"})
+        self.assertEqual(
+            "application/json",
+            api.get_response_content_type(response))
+
+    def test_get_response_content_type_return_None_when_type_not_found(self):
+        response = httplib2.Response({})
+        self.assertIsNone(api.get_response_content_type(response))
+
+
+class TestIsResponseTextual(TestCase):
+    """Tests for `is_response_textual`."""
+
+    content_types = {
+        "text/plain": True,
+        "text/yaml": True,
+        "text/foobar": True,
+        "application/json": True,
+        "image/png": False,
+        "video/webm": False,
+        }
+
+    scenarios = (
+        (ctype, {"content_type": ctype, "is_textual": is_textual})
+        for ctype, is_textual in content_types.items()
+        )
+
+    def test_type(self):
+        grct = self.patch(api, "get_response_content_type")
+        grct.return_value = self.content_type
+        self.assertEqual(
+            self.is_textual,
+            api.is_response_textual(sentinel.response))
+        grct.assert_called_once_with(sentinel.response)
+
 
 class TestAction(TestCase):
     """Tests for :class:`maascli.api.Action`."""