Improve API of 'git_pw.api' module

There were too many public things being exposed by this module. Clean
things up a little.

Signed-off-by: Stephen Finucane <stephen@that.guru>

Author: stephenfin

git_pw/api.py

@@ -6,6 +6,7 @@
 import logging
 import os.path
 import re
+import pty
 import sys
 import tempfile
 
@@ -17,6 +18,7 @@
 
 if 0:  # noqa
     from typing import Dict  # noqa
+    from typing import IO  # noqa
     from typing import List  # noqa
     from typing import Optional  # noqa
     from typing import Tuple  # noqa
@@ -121,20 +123,7 @@ def _handle_error(operation, exc):
         sys.exit(1)
 
 
-def version():
-    # type: () -> Optional[Tuple[int, int]]
-    """Get the version of the server from the URL, if present."""
-    server = _get_server()
-
-    version = re.match(r'.*/(\d)\.(\d)$', server)
-    if version:
-        return (int(version.group(1)), int(version.group(2)))
-
-    # return the oldest version we support if no version provided
-    return (1, 0)
-
-
-def get(url, params=None, stream=False):
+def _get(url, params=None, stream=False):
     # type: (str, Filters, bool) -> requests.Response
     """Make GET request and handle errors."""
     LOG.debug('GET %s', url)
@@ -155,7 +144,7 @@ def get(url, params=None, stream=False):
     return rsp
 
 
-def patch(url, data):
+def _patch(url, data):
     # type: (str, dict) -> requests.Response
     """Make PATCH request and handle errors."""
     LOG.debug('PATCH %s, data=%r', url, data)
@@ -172,39 +161,64 @@ def patch(url, data):
     return rsp
 
 
-def download(url, params=None):
-    # type: (str, Filters) -> str
-    """Retrieve a specific API resource and save it to a file.
+def version():
+    # type: () -> Optional[Tuple[int, int]]
+    """Get the version of the server from the URL, if present."""
+    server = _get_server()
 
-    GET /{resource}/{resourceID}/
+    version = re.match(r'.*/(\d)\.(\d)$', server)
+    if version:
+        return (int(version.group(1)), int(version.group(2)))
+
+    # return the oldest version we support if no version provided
+    return (1, 0)
+
+
+def download(url, params=None, output=None):
+    # type: (str, Filters, IO) -> Optional[str]
+    """Retrieve a specific API resource and save it to a file/stdout.
 
     The ``Content-Disposition`` header is assumed to be present and
-    will be used for the output filename.
+    will be used for the output filename, if not writing to stdout.
 
     Arguments:
         url: The resource URL.
         params: Additional parameters.
+        output: The output file. If provided, the caller is responsible for
+            closing. If None, a temporary file will be used.
 
     Returns:
-        A path to an output file containing the content.
+        A path to an output file containing the content, else None if stdout
+        used.
     """
-    rsp = get(url, params, stream=True)
+    rsp = _get(url, params, stream=True)
 
     # we don't catch anything here because we should break if these are missing
-    header = re.search('filename=(.+)',
-                       rsp.headers.get('content-disposition') or '')
+    header = re.search(
+        'filename=(.+)', rsp.headers.get('content-disposition') or '',
+    )
     if not header:
         LOG.error('Filename was expected but was not provided in response')
         sys.exit(1)
 
-    output_path = os.path.join(tempfile.mkdtemp(prefix='git-pw'),
-                               header.group(1))
+    if output:
+        output_path = None
+        if output.fileno() != pty.STDOUT_FILENO:
+            LOG.debug('Saving to %s', output.name)
+            output_path = output.name
 
-    with open(output_path, 'wb') as output_file:
-        LOG.debug('Saving to %s', output_path)
         # we use iter_content because patches can be binary
         for block in rsp.iter_content(1024):
-            output_file.write(block)
+            output.write(block)
+    else:
+        output_path = os.path.join(
+            tempfile.mkdtemp(prefix='git-pw'), header.group(1),
+        )
+        with open(output_path, 'wb') as output_file:
+            LOG.debug('Saving to %s', output_path)
+            # we use iter_content because patches can be binary
+            for block in rsp.iter_content(1024):
+                output_file.write(block)
 
     return output_path
 
@@ -233,7 +247,7 @@ def index(resource_type, params=None):
     params = params or []
     params.append(('project', _get_project()))
 
-    return get(url, params).json()
+    return _get(url, params).json()
 
 
 def detail(resource_type, resource_id, params=None):
@@ -253,7 +267,7 @@ def detail(resource_type, resource_id, params=None):
     # NOTE(stephenfin): All resources must have a trailing '/'
     url = '/'.join([_get_server(), resource_type, str(resource_id), ''])
 
-    return get(url, params, stream=False).json()
+    return _get(url, params, stream=False).json()
 
 
 def update(resource_type, resource_id, data):
@@ -273,7 +287,7 @@ def update(resource_type, resource_id, data):
     # NOTE(stephenfin): All resources must have a trailing '/'
     url = '/'.join([_get_server(), resource_type, str(resource_id), ''])
 
-    return patch(url, data).json()
+    return _patch(url, data).json()
 
 
 def validate_multiple_filter_support(f):

git_pw/bundle.py

@@ -3,7 +3,6 @@
 """
 
 import logging
-import pty
 import sys
 
 import click
@@ -72,15 +71,7 @@ def download_cmd(bundle_id, output):
     path = None
     bundle = _get_bundle(bundle_id)
 
-    if output:
-        content = api.get(bundle['mbox']).content
-
-        output.write(content)
-
-        if output.fileno() != pty.STDOUT_FILENO:
-            path = output.name
-    else:
-        path = api.download(bundle['mbox'])
+    path = api.download(bundle['mbox'], output=output)
 
     if path:
         LOG.info('Downloaded bundle to %s', path)

git_pw/patch.py

@@ -75,24 +75,20 @@ def download_cmd(patch_id, output, fmt):
     path = None
     patch = api.detail('patches', patch_id)
 
-    if output:
-        if fmt == 'diff':
-            content = patch['diff']
+    if fmt == 'diff':
+        if output:
+            output.write(patch['diff'])
+            if output.fileno() != pty.STDOUT_FILENO:
+                path = output.name
         else:
-            content = api.get(patch['mbox']).content
-
-        output.write(content)
-
-        if output.fileno() != pty.STDOUT_FILENO:
-            path = output.name
-    else:
-        if fmt == 'diff':
             # TODO(stephenfin): We discard the 'diff' field so we can get the
             # filename and save to the correct file. We should expose this
             # information via the API
-            path = api.download(patch['mbox'].replace('mbox', 'raw'))
-        else:
-            path = api.download(patch['mbox'])
+            path = api.download(
+                patch['mbox'].replace('mbox', 'raw'), output=output,
+            )
+    else:
+        path = api.download(patch['mbox'], output=output)
 
     if path:
         LOG.info('Downloaded patch to %s', path)

git_pw/series.py

@@ -3,7 +3,6 @@
 """
 
 import logging
-import pty
 
 import arrow
 import click
@@ -51,15 +50,7 @@ def download_cmd(series_id, output):
     path = None
     series = api.detail('series', series_id)
 
-    if output:
-        content = api.get(series['mbox']).content
-
-        output.write(content)
-
-        if output.fileno() != pty.STDOUT_FILENO:
-            path = output.name
-    else:
-        path = api.download(series['mbox'])
+    path = api.download(series['mbox'], output=output)
 
     if path:
         LOG.info('Downloaded series to %s', path)

tests/test_bundle.py

@@ -1,6 +1,7 @@
 import unittest
 
 from click.testing import CliRunner as CLIRunner
+from click import utils as click_utils
 import mock
 
 from git_pw import bundle
@@ -94,10 +95,9 @@ def test_apply_with_args(self, mock_git_am, mock_download,
 
 @mock.patch('git_pw.bundle._get_bundle')
 @mock.patch('git_pw.api.download')
-@mock.patch('git_pw.api.get')
 class DownloadTestCase(unittest.TestCase):
 
-    def test_download(self, mock_get, mock_download, mock_get_bundle):
+    def test_download(self, mock_download, mock_get_bundle):
         """Validate standard behavior."""
 
         rsp = {'mbox': 'http://example.com/api/patches/123/mbox/'}
@@ -109,33 +109,24 @@ def test_download(self, mock_get, mock_download, mock_get_bundle):
 
         assert result.exit_code == 0, result
         mock_get_bundle.assert_called_once_with('123')
-        mock_download.assert_called_once_with(rsp['mbox'])
-        mock_get.assert_not_called()
+        mock_download.assert_called_once_with(rsp['mbox'], output=None)
 
-    def test_download_to_file(self, mock_get, mock_download, mock_get_bundle):
+    def test_download_to_file(self, mock_download, mock_get_bundle):
         """Validate downloading to a file."""
 
-        class MockResponse(object):
-            @property
-            def content(self):
-                return b'alpha-beta'
-
         rsp = {'mbox': 'http://example.com/api/patches/123/mbox/'}
         mock_get_bundle.return_value = rsp
-        mock_get.return_value = MockResponse()
 
         runner = CLIRunner()
-        with runner.isolated_filesystem():
-            result = runner.invoke(bundle.download_cmd, ['123', 'test.patch'])
+        result = runner.invoke(bundle.download_cmd, ['123', 'test.patch'])
 
-            assert result.exit_code == 0, result
-
-            with open('test.patch') as output:
-                assert ['alpha-beta'] == output.readlines()
+        assert result.exit_code == 0, result
 
         mock_get_bundle.assert_called_once_with('123')
-        mock_get.assert_called_once_with(rsp['mbox'])
-        mock_download.assert_not_called()
+        mock_download.assert_called_once_with(rsp['mbox'], output=mock.ANY)
+        assert isinstance(
+            mock_download.call_args[1]['output'], click_utils.LazyFile,
+        )
 
 
 class ShowTestCase(unittest.TestCase):

tests/test_patch.py

@@ -2,6 +2,7 @@
 
 import click
 from click.testing import CliRunner as CLIRunner
+from click import utils as click_utils
 import mock
 from packaging import version
 
@@ -92,7 +93,7 @@ def test_download(self, mock_log, mock_download, mock_detail):
 
         assert result.exit_code == 0, result
         mock_detail.assert_called_once_with('patches', 123)
-        mock_download.assert_called_once_with(rsp['mbox'])
+        mock_download.assert_called_once_with(rsp['mbox'], output=None)
         assert mock_log.info.called
 
     def test_download_diff(self, mock_log, mock_download, mock_detail):
@@ -106,42 +107,30 @@ def test_download_diff(self, mock_log, mock_download, mock_detail):
 
         assert result.exit_code == 0, result
         mock_detail.assert_called_once_with('patches', 123)
-        mock_download.assert_called_once_with(rsp['mbox'].replace(
-            'mbox', 'raw'))
+        mock_download.assert_called_once_with(
+            rsp['mbox'].replace('mbox', 'raw'), output=None,
+        )
         assert mock_log.info.called
 
-    @mock.patch('git_pw.api.get')
-    def test_download_to_file(self, mock_get, mock_log, mock_download,
-                              mock_detail):
+    def test_download_to_file(self, mock_log, mock_download, mock_detail):
         """Validate behavior if downloading to a specific file."""
 
-        class MockResponse(object):
-            @property
-            def content(self):
-                return b'alpha-beta'
-
         rsp = {'mbox': 'hello, world', 'diff': 'test'}
         mock_detail.return_value = rsp
-        mock_get.return_value = MockResponse()
 
         runner = CLIRunner()
-        with runner.isolated_filesystem():
-            result = runner.invoke(patch.download_cmd,
-                                   ['123', 'test.patch'])
+        result = runner.invoke(patch.download_cmd, ['123', 'test.patch'])
 
-            assert result.exit_code == 0, result
-
-            with open('test.patch') as output:
-                assert ['alpha-beta'] == output.readlines()
+        assert result.exit_code == 0, result
 
         mock_detail.assert_called_once_with('patches', 123)
-        mock_download.assert_not_called()
-        mock_get.assert_called_once_with(rsp['mbox'])
+        mock_download.assert_called_once_with(rsp['mbox'], output=mock.ANY)
+        assert isinstance(
+            mock_download.call_args[1]['output'], click_utils.LazyFile,
+        )
         assert mock_log.info.called
 
-    @mock.patch('git_pw.api.get')
-    def test_download_diff_to_file(self, mock_get, mock_log, mock_download,
-                                   mock_detail):
+    def test_download_diff_to_file(self, mock_log, mock_download, mock_detail):
         """Validate behavior if downloading a diff to a specific file."""
 
         rsp = {'mbox': 'hello, world', 'diff': b'test'}
@@ -159,7 +148,6 @@ def test_download_diff_to_file(self, mock_get, mock_log, mock_download,
 
         mock_detail.assert_called_once_with('patches', 123)
         mock_download.assert_not_called()
-        mock_get.assert_not_called()
         assert mock_log.info.called