Save to file by default

Usability testing by a colleague identified this as a confusing aspect
of the UX. For most people, download roughly equates to "save to a
file", not "output to stdout". Resolve this by downloading to a file
automatically unless specifically requests. This also requires bumping
the logging level to INFO to allow us to output helpful information.

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

Author: stephenfin

git_pw/api.py

@@ -3,9 +3,8 @@
 """
 
 import logging
-import os
+import re
 import sys
-import tempfile
 
 import requests
 
@@ -138,17 +137,23 @@ def download(url, params=None):
 
     GET /{resource}/{resourceID}/
 
+    The ``Content-Disposition`` header is assumed to be present and
+    will be used for the output filename.
+
     Arguments:
         url: The resource URL.
         params: Additional parameters.
 
     Returns:
         A path to an output file containing the content.
     """
-    output_fd, output_path = tempfile.mkstemp(suffix='.patch')
-
     rsp = get(url, params, stream=True)
-    with os.fdopen(output_fd, 'wb') as output_file:
+
+    # we don't catch anything here because we should break if these are missing
+    header = rsp.headers['content-disposition']
+    output_path = re.search('filename=(.+)', 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):

git_pw/bundle.py

@@ -32,17 +32,29 @@ def apply_cmd(bundle_id, args):
 
 @click.command(name='download')
 @click.argument('bundle_id', type=click.INT)
-def download_cmd(bundle_id):
+@click.argument('output', type=click.File('wb'), required=False)
+def download_cmd(bundle_id, output):
     """Download bundle in mbox format.
 
-    Download a bundle but do not apply it.
+    Download a bundle but do not apply it. ``OUTPUT`` is optional and can be an
+    output path or ``-`` to output to ``stdout``. If ``OUTPUT`` is not
+    provided, the output path will be automatically chosen.
     """
     LOG.debug('Downloading bundle: id=%d', bundle_id)
 
+    path = None
     bundle = api.detail('bundles', bundle_id)
-    output = api.get(bundle['mbox']).text
 
-    click.echo_via_pager(output)
+    if output:
+        output.write(api.get(bundle['mbox']).text)
+
+        if output != sys.stdout:
+            path = output.name
+    else:
+        path = api.download(bundle['mbox'])
+
+    if path:
+        LOG.info('Downloaded bundle to %s', path)
 
 
 @click.command(name='show')

git_pw/logger.py

@@ -11,4 +11,4 @@ def configure_verbosity(debug):  # type: (bool) -> None
             level=logging.DEBUG,
             format='%(asctime)s - %(name)s - %(levelname)s - %(message)s')
     else:
-        logging.basicConfig(level=logging.ERROR, format='%(message)s')
+        logging.basicConfig(level=logging.INFO, format='%(message)s')

git_pw/patch.py

@@ -45,25 +45,44 @@ def apply_cmd(patch_id, series, deps, args):
 
 @click.command(name='download')
 @click.argument('patch_id', type=click.INT)
+@click.argument('output', type=click.File('wb'), required=False)
 @click.option('--diff', 'fmt', flag_value='diff',
               help='Show patch in diff format.')
 @click.option('--mbox', 'fmt', flag_value='mbox', default=True,
               help='Show patch in mbox format.')
-def download_cmd(patch_id, fmt):
-    """Download a patch diff/mbox.
+def download_cmd(patch_id, output, fmt):
+    """Download patch in diff or mbox format.
 
-    Download a patch but do not apply it.
+    Download a patch but do not apply it. ``OUTPUT`` is optional and can be an
+    output path or ``-`` to output to ``stdout``. If ``OUTPUT`` is not
+    provided, the output path will be automatically chosen.
     """
     LOG.debug('Downloading patch: id=%d, format=%s', patch_id, fmt)
 
+    path = None
     patch = api.detail('patches', patch_id)
 
-    if fmt == 'diff':
-        output = patch['diff']
-    else:
-        output = api.get(patch['mbox']).text
+    if output:
+        if fmt == 'diff':
+            content = patch['diff']
+        else:
+            content = api.get(patch['mbox']).text
+
+        output.write(content)
 
-    click.echo_via_pager(output)
+        if output != sys.stdout:
+            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'])
+
+    if path:
+        LOG.info('Downloaded patch to %s', path)
 
 
 def _show_patch(patch):

git_pw/series.py

@@ -33,17 +33,29 @@ def apply_cmd(series_id, args):
 
 @click.command(name='download')
 @click.argument('series_id', type=click.INT)
-def download_cmd(series_id):
+@click.argument('output', type=click.File('wb'), required=False)
+def download_cmd(series_id, output):
     """Download series in mbox format.
 
-    Download a series but do not apply it.
+    Download a series but do not apply it. ``OUTPUT`` is optional and can be an
+    output path or ``-`` to output to ``stdout``. If ``OUTPUT`` is not
+    provided, the output path will be automatically chosen.
     """
     LOG.debug('Downloading series: id=%d', series_id)
 
+    path = None
     series = api.detail('series', series_id)
-    output = api.get(series['mbox']).text
 
-    click.echo_via_pager(output)
+    if output:
+        output.write(api.get(series['mbox']).text)
+
+        if output != sys.stdout:
+            path = output.name
+    else:
+        path = api.download(series['mbox'])
+
+    if path:
+        LOG.info('Downloaded series to %s', path)
 
 
 @click.command(name='show')

releasenotes/notes/save-patches-to-file-c667ab7dd0b73ead.yaml

@@ -0,0 +1,20 @@
+---
+features:
+  - |
+    Patches, series and bundles downloaded using the ``download`` command will
+    now be saved to a file by default instead of output to stdout. By default,
+    this will use the name provided by Patchwork but you can override this by
+    passing a specific filename. For example::
+
+        $ git pw patch download 1234 hello-world.patch
+
+    You can also output to ``stdout`` using the ``-`` shortcut. For example::
+
+        $ git pw patch download 1234 -
+upgrade:
+  - |
+    Patches, series and bundles downloaded using the ``download`` command will
+    now be saved to a file by default. To continue outputing to ``stdout``,
+    use ``-``. For example::
+
+        $ git pw patch download 1234 -

tox.ini

@@ -23,7 +23,7 @@ commands=
 
 [testenv:docs]
 commands =
-    python setup.py build_sphinx
+    python setup.py build_sphinx {posargs}
 
 [testenv:man]
 commands =