Misc review bugs

isic_cli/cli/__init__.py

@@ -2,7 +2,6 @@
 
 from contextlib import contextmanager
 import datetime
-from http.client import HTTPConnection
 import logging
 import os
 import platform
@@ -127,10 +126,9 @@ def cli(ctx, verbose: bool, guest: bool, sandbox: bool, dev: bool, no_version_ch
     logger.setLevel(logging.WARNING)
 
     if verbose:
-        HTTPConnection.debuglevel = 1
-        requests_log = logging.getLogger("requests.packages.urllib3")
-        requests_log.addHandler(logging.StreamHandler(sys.stderr))
-        requests_log.setLevel(logging.DEBUG)
+        urllib3_log = logging.getLogger("urllib3")
+        urllib3_log.addHandler(logging.StreamHandler(sys.stderr))
+        urllib3_log.setLevel(logging.DEBUG)
         logger.setLevel(logging.DEBUG)
 
     if sandbox and dev:
@@ -163,27 +161,29 @@ def cli(ctx, verbose: bool, guest: bool, sandbox: bool, dev: bool, no_version_ch
             click.secho(
                 "Something went wrong with restoring a login, you may need to log back in.",
                 fg="yellow",
+                err=True,
             )
 
-    with get_session(f"{DOMAINS[env]}/api/v2/", oauth.auth_headers) as session:
-        user = None
-        if oauth.auth_headers:
-            try:
-                user = get_users_me(session)
-            except HTTPError as e:
-                if e.response.status_code == 404:
-                    # perhaps a stale token
-                    oauth.logout()
-                else:
-                    raise
-
-        ctx.obj = IsicContext(
-            oauth=oauth,
-            session=session,
-            env=env,
-            user=user,
-            verbose=verbose,
-        )
+    session = ctx.with_resource(get_session(f"{DOMAINS[env]}/api/v2/", oauth.auth_headers))
+
+    user = None
+    if oauth.auth_headers:
+        try:
+            user = get_users_me(session)
+        except HTTPError as e:
+            if e.response.status_code == 404:
+                # perhaps a stale token
+                oauth.logout()
+            else:
+                raise
+
+    ctx.obj = IsicContext(
+        oauth=oauth,
+        session=session,
+        env=env,
+        user=user,
+        verbose=verbose,
+    )
 
 
 cli.add_command(accession_group, name="accession")

isic_cli/cli/collection.py

@@ -23,6 +23,10 @@
 
 def _parse_isic_ids(ctx, param, value) -> list[str]:
     isic_ids = {line.strip() for line in value.read().splitlines() if line.strip() != ""}
+    if not isic_ids:
+        click.secho("No ISIC IDs were provided.", err=True, fg="red")
+        sys.exit(1)
+
     for isic_id in isic_ids:
         if not re.match(r"^ISIC_\d{7}$", isic_id):
             click.secho(f'Found invalidly formatted ISIC ID: "{isic_id}"', err=True, fg="red")
@@ -41,7 +45,7 @@ def _table_from_summary(summary: dict[str, list[str]], nice_map: dict | None = N
 
     def examples(isic_ids: list) -> str:
         s = set(isic_ids)
-        ret = ", ".join(sorted(list(s)[:3]))
+        ret = ", ".join(sorted(s)[:3])
         if len(s) > 3:
             ret += ", etc."
         else:
@@ -54,7 +58,7 @@ def examples(isic_ids: list) -> str:
 
     for k, v in summary.items():
         if k != "succeeded":  # already printed
-            table.add_row(nice_map[k], str(len(v)), examples(v))
+            table.add_row(nice_map.get(k, k), str(len(v)), examples(v))
 
     return table
 

isic_cli/cli/image.py

@@ -84,9 +84,7 @@ def image(ctx):
     pass
 
 
-@image.command(
-    name="download", help="Download a set of images and metadata, optionally filtering results."
-)
+@image.command(name="download")
 @click.option(
     "-s",
     "--search",
@@ -126,7 +124,7 @@ def download(
     outdir: Path,
 ):
     """
-    Download images from the ISIC Archive.
+    Download a set of images and metadata, optionally filtering results.
 
     The search query uses a simple DSL syntax.
 
@@ -153,15 +151,11 @@ def download(
 
     outdir.mkdir(parents=True, exist_ok=True)
 
-    def signal_handler(signum, frame):
-        cleanup_partially_downloaded_files(outdir)
-        sys.exit(1)
-
-    # remove partially downloaded files on exit
+    # remove partially downloaded files on exit. this runs after the download threads have
+    # finished, so it can't remove a file that's still being written.
     atexit.register(cleanup_partially_downloaded_files, outdir)
-    # also remove partially downloaded files on SIGINT/SIGTERM
-    signal.signal(signal.SIGINT, signal_handler)
-    signal.signal(signal.SIGTERM, signal_handler)
+    # SIGTERM exits without running atexit handlers by default, so handle it like SIGINT
+    signal.signal(signal.SIGTERM, signal.default_int_handler)
 
     archive_num_images = get_num_images(ctx.session, search, collections)
     download_num_images = archive_num_images if limit == 0 else min(archive_num_images, limit)
@@ -193,7 +187,8 @@ def signal_handler(signum, frame):
         with ThreadPoolExecutor(max(10, os.cpu_count() or 10)) as thread_pool:
             for image_chunk in chunked(images_iterator, 100):
                 images.extend(image_chunk)
-                thread_pool.map(func, image_chunk)
+                # consume the results so a failed download raises instead of being ignored
+                list(thread_pool.map(func, image_chunk))
 
         headers, records = _extract_metadata(images)
         with (outdir / "metadata.csv").open("w", newline="", encoding="utf8") as outfile:
@@ -208,8 +203,8 @@ def signal_handler(signum, frame):
         licenses = {record["copyright_license"] for record in records}
         (outdir / "licenses").mkdir(exist_ok=True)
         for license_type in licenses:
-            with (outdir / "licenses" / f"{license_type}.txt").open("w") as outfile:
-                outfile.write(get_license(ctx.session, license_type))
+            license_text = get_license(ctx.session, license_type)
+            (outdir / "licenses" / f"{license_type}.txt").write_text(license_text, encoding="utf8")
 
     click.echo()
     click.secho(f"Successfully downloaded {nice_num_images} images to {outdir}/.", fg="green")

isic_cli/cli/metadata.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 from collections import defaultdict
+import contextlib
 import csv
 import itertools
 import os
@@ -211,12 +212,12 @@ def download(
         )
         headers, records = _extract_metadata(images, progress, task)
 
-    if records:
+    with contextlib.ExitStack() as stack:
         if outfile is None or os.fsdecode(outfile) == "-":
             sys.stdout.reconfigure(encoding="utf8")
             stream = sys.stdout
         else:
-            stream = Path(outfile).open("w", newline="", encoding="utf8")  # noqa: SIM115
+            stream = stack.enter_context(Path(outfile).open("w", newline="", encoding="utf8"))
 
         writer = csv.DictWriter(stream, headers)
         writer.writeheader()

isic_cli/cli/types.py

@@ -34,6 +34,7 @@ def convert(self, value, param, ctx):
         r = ctx.obj.session.get("images/search/", params={"query": value, "limit": 1})
         if r.status_code == 400 and "message" in r.json() and "query" in r.json()["message"]:
             self.fail(f'Invalid search query string "{value}"', param, ctx)
+        r.raise_for_status()
         return value
 
 

isic_cli/cli/utils.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 from collections import Counter
+import functools
 import sys
 from typing import TYPE_CHECKING
 
@@ -13,18 +14,20 @@
 
 
 def suggest_guest_login(f):
+    @functools.wraps(f)
     def decorator(ctx: IsicContext, **kwargs):
         if not ctx.user:
             click.echo(
                 "If you have been granted special permissions, logging in with `isic user login` might return more data.\n",  # noqa: E501
                 err=True,
             )
-        f(ctx, **kwargs)
+        return f(ctx, **kwargs)
 
     return decorator
 
 
 def require_login(f):
+    @functools.wraps(f)
     def decorator(ctx: IsicContext, **kwargs):
         if not ctx.user:
             click.echo(
@@ -33,7 +36,7 @@ def decorator(ctx: IsicContext, **kwargs):
             )
             sys.exit(1)
 
-        f(ctx, **kwargs)
+        return f(ctx, **kwargs)
 
     return decorator
 

isic_cli/io/http.py

@@ -6,7 +6,7 @@
 import shutil
 from tempfile import NamedTemporaryFile
 from typing import TYPE_CHECKING
-from urllib.parse import urlparse
+from urllib.parse import urlencode, urlparse
 
 from more_itertools import chunked
 from requests.exceptions import ChunkedEncodingError, ConnectionError
@@ -93,7 +93,7 @@ def bulk_collection_operation(  # noqa: PLR0913
 
 
 def get_images(session: IsicCliSession, search: str = "", collections: str = "") -> Iterable[dict]:
-    next_page = f"images/search/?query={search}&collections={collections}"
+    next_page = f"images/search/?{urlencode({'query': search, 'collections': collections})}"
 
     while next_page:
         r = session.get(next_page)

isic_cli/utils/version.py

@@ -5,6 +5,7 @@
 import sys
 
 import click
+from packaging.utils import parse_sdist_filename, parse_wheel_filename
 from packaging.version import Version
 import requests
 from requests.exceptions import RequestException
@@ -26,6 +27,8 @@ def is_dev_install():
 
 
 def upgrade_type(from_version: Version, to_version: Version) -> str | None:
+    if to_version <= from_version:
+        return None
     if to_version.major > from_version.major:
         return "major"
     if to_version.minor > from_version.minor:
@@ -34,14 +37,30 @@ def upgrade_type(from_version: Version, to_version: Version) -> str | None:
         return "micro"
 
 
-def _pypi_releases():
-    r = requests.get("https://pypi.org/pypi/isic-cli/json", timeout=(5, 5))
+def _pypi_files() -> list[dict]:
+    # https://peps.python.org/pep-0691/
+    r = requests.get(
+        "https://pypi.org/simple/isic-cli/",
+        headers={"Accept": "application/vnd.pypi.simple.v1+json"},
+        timeout=(5, 5),
+    )
     r.raise_for_status()
-    return r.json()["releases"]
+    return r.json()["files"]
+
+
+def _file_version(filename: str) -> Version:
+    if filename.endswith(".whl"):
+        return parse_wheel_filename(filename)[1]
+    return parse_sdist_filename(filename)[1]
 
 
 def newest_version_available() -> Version | None:
-    releases = [Version(v) for v in _pypi_releases()]
+    releases = [
+        _file_version(file["filename"])
+        for file in _pypi_files()
+        # yanked is optional, and is either a boolean or the reason the file was yanked
+        if not file.get("yanked")
+    ]
     real_releases = [x for x in releases if not x.is_prerelease and not x.is_devrelease]
     if real_releases:
         return sorted(real_releases)[-1]

tests/test_cli_base.py

@@ -123,3 +123,11 @@ def test_connection_error_is_not_reported_as_bug(mocker, capsys):
     assert "Unable to connect to the ISIC Archive" in capsys.readouterr().err
     prompt.assert_not_called()
     assert spy.call_count == 0
+
+
+@pytest.mark.parametrize("command", ["image", "metadata"])
+def test_download_help(cli_run, command):
+    result = cli_run([command, "download", "--help"])
+
+    assert result.exit_code == 0, result.exception
+    assert "The search query uses a simple DSL syntax." in result.output

tests/test_cli_collection.py

@@ -110,19 +110,35 @@ def test_collection_add_images(cli_run, mocker):
             "doi": None,
         },
     )
+    succeeded = [f"ISIC_{i:07d}" for i in reversed(range(4))]
     mocker.patch(
         "isic_cli.cli.collection.bulk_collection_operation",
         return_value={
-            "succeeded": ["ISIC_0000000"],
+            "succeeded": succeeded,
             "no_perms_or_does_not_exist": ["ISIC_1234567"],
             "private_image_public_collection": ["ISIC_1111111"],
+            "some_new_status": ["ISIC_2222222"],
         },
     )
 
     result = cli_run(
         ["collection", "add-images", "1", "--from-isic-ids", "-"],
         input="ISIC_1111111\nISIC_1234567",
+        # keep the examples column from wrapping
+        env={"COLUMNS": "200"},
     )
 
     assert result.exit_code == 0, (result.exception, result.output)
-    assert re.search(r"Image added.*ISIC_0000000", result.output), result.output
+    examples = ", ".join(sorted(succeeded)[:3])
+    assert re.search(rf"Image added.*{examples}, etc\.", result.output), result.output
+    assert re.search(r"some_new_status.*ISIC_2222222", result.output), result.output
+
+
+@pytest.mark.usefixtures("_mock_user")
+def test_collection_add_images_empty_input(cli_run, mocker):
+    mocker.patch("isic_cli.cli.types.get_collection", return_value={"locked": False})
+
+    result = cli_run(["collection", "add-images", "1", "--from-isic-ids", "-"], input="\n")
+
+    assert result.exit_code == 1, (result.exception, result.output)
+    assert "No ISIC IDs were provided." in result.output