From d2ec7ee16d838e6b0e3548ebb063af997a61f4fa Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:28:15 -0400 Subject: [PATCH 01/11] Fail image downloads when an image can't be downloaded The iterator returned by ThreadPoolExecutor.map was discarded, so any exception raised by download_image (an HTTP error, or running out of retries) was never seen. The command reported that every image was downloaded and wrote metadata for images that weren't. Consuming the results raises the first failure. It also means each chunk finishes downloading before the next page of images is fetched, instead of the entire result set being queued up front. Since map cancels its pending futures when the iterator is interrupted, Ctrl-C now only waits for the downloads in progress. Before, it waited for every remaining image to download. --- isic_cli/cli/image.py | 3 ++- tests/test_cli_image.py | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/isic_cli/cli/image.py b/isic_cli/cli/image.py index f3dc34a..cee608a 100644 --- a/isic_cli/cli/image.py +++ b/isic_cli/cli/image.py @@ -193,7 +193,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: diff --git a/tests/test_cli_image.py b/tests/test_cli_image.py index 69d684e..0bad4c8 100644 --- a/tests/test_cli_image.py +++ b/tests/test_cli_image.py @@ -54,6 +54,20 @@ def test_image_download(cli_run, outdir): assert Path(f"{outdir}/licenses/CC-0.txt").exists() +@pytest.mark.usefixtures("_isolated_filesystem", "_mock_images") +def test_image_download_failure(mocker, cli_run, outdir): + mocker.patch( + "isic_cli.cli.image.download_image", + side_effect=HTTPError(response=mocker.MagicMock(status_code=403)), + ) + + result = cli_run(["image", "download", outdir]) + + assert result.exit_code == 1, result.exception + assert "Successfully downloaded" not in result.output + assert not Path(f"{outdir}/metadata.csv").exists() + + @pytest.mark.usefixtures("_isolated_filesystem", "_mock_images") def test_image_download_no_collection(mocker, cli_run, outdir): mocker.patch( From 28633791da7fe17edf6cfb93c996d6902bd22866 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:32:18 -0400 Subject: [PATCH 02/11] Don't delete partial downloads that are still being written The SIGINT/SIGTERM handler removed every .isic-partial file before exiting, including the ones worker threads were still writing. Those downloads then failed when moving the file into place, so every image in progress when Ctrl-C was pressed was thrown away. On Windows the open files can't be removed at all, which is likely where the "Permission error while cleaning up" warnings came from. Leave the cleanup to the atexit handler, which runs after the worker threads have finished. SIGINT already raises KeyboardInterrupt, and SIGTERM now does the same so that atexit handlers still run. --- isic_cli/cli/image.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/isic_cli/cli/image.py b/isic_cli/cli/image.py index cee608a..0188e64 100644 --- a/isic_cli/cli/image.py +++ b/isic_cli/cli/image.py @@ -153,15 +153,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) From 8a060607ee1c6f7dcddc76e79840597fb169e536 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:32:58 -0400 Subject: [PATCH 03/11] URL-encode the search when listing images get_images interpolated the search into the URL without encoding it, so a query containing & was split into separate parameters, and a + was read by the server as a space. get_num_images passes the same search through params= and encodes it correctly, so the reported count and the images actually downloaded could disagree. --- isic_cli/io/http.py | 4 ++-- tests/test_http.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 tests/test_http.py diff --git a/isic_cli/io/http.py b/isic_cli/io/http.py index 5d60eee..7a84b47 100644 --- a/isic_cli/io/http.py +++ b/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) diff --git a/tests/test_http.py b/tests/test_http.py new file mode 100644 index 0000000..24a7cf1 --- /dev/null +++ b/tests/test_http.py @@ -0,0 +1,16 @@ +from __future__ import annotations + +from urllib.parse import parse_qs, urlparse + +from isic_cli.io.http import get_images + + +def test_get_images_encodes_search(mocker): + session = mocker.MagicMock() + session.get.return_value.json.return_value = {"results": [], "next": None} + search = 'attribution:"Foo & Bar" AND age_approx:[5 TO 25] AND sex:male+' + + list(get_images(session, search=search, collections="1,2")) + + url = session.get.call_args.args[0] + assert parse_qs(urlparse(url).query) == {"query": [search], "collections": ["1,2"]} From 3d943ad61a518889508912e4b20d3f4e548bb6f1 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:34:04 -0400 Subject: [PATCH 04/11] Reject an empty list of ISIC IDs when adding or removing images An empty --from-isic-ids file meant no requests were made, so the summary had no "succeeded" key and building the results table raised a KeyError, which was reported as a bug in isic-cli. While here, label any status the server adds in the future with its raw name instead of raising a KeyError after the images have already been changed, and show the first three examples in sorted order. Three arbitrary IDs were picked from a set before sorting. --- isic_cli/cli/collection.py | 8 ++++++-- tests/test_cli_collection.py | 20 ++++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/isic_cli/cli/collection.py b/isic_cli/cli/collection.py index 0064b9b..a625a70 100644 --- a/isic_cli/cli/collection.py +++ b/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 diff --git a/tests/test_cli_collection.py b/tests/test_cli_collection.py index f5a9d59..c1bf18a 100644 --- a/tests/test_cli_collection.py +++ b/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 From 441f812960ff3d7f99c9ee223d78cc0cf5bcc0c9 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:34:42 -0400 Subject: [PATCH 05/11] Show the docstring in help for commands that check the login suggest_guest_login and require_login didn't use functools.wraps, so click.pass_obj copied the wrapper's empty docstring and "isic metadata download --help" showed no description. They also dropped the command's return value. image download passed help=, which hid its docstring and the example search queries in it. Move that text into the docstring instead. --- isic_cli/cli/image.py | 6 ++---- isic_cli/cli/utils.py | 7 +++++-- tests/test_cli_base.py | 8 ++++++++ 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/isic_cli/cli/image.py b/isic_cli/cli/image.py index 0188e64..123c251 100644 --- a/isic_cli/cli/image.py +++ b/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. diff --git a/isic_cli/cli/utils.py b/isic_cli/cli/utils.py index 8dadd9b..136df22 100644 --- a/isic_cli/cli/utils.py +++ b/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 diff --git a/tests/test_cli_base.py b/tests/test_cli_base.py index ab62eb8..b3f6f88 100644 --- a/tests/test_cli_base.py +++ b/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 From ef9f445636292c93573a76b6a80062fe9f31a73a Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:35:46 -0400 Subject: [PATCH 06/11] Keep warnings and verbose output off stdout Commands like "isic metadata download > out.csv" write their results to stdout, but two things could end up in the output: - The warning about failing to restore a login was printed to stdout. - -v set HTTPConnection.debuglevel, and http.client prints its debug output to stdout. That output also included the Authorization header. Print the warning to stderr, and log requests through urllib3 instead, which goes to stderr. The urllib3 logging -v tried to enable never worked, since requests.packages.urllib3 is an alias of the urllib3 module and its loggers are named "urllib3.*". --- isic_cli/cli/__init__.py | 9 ++++----- tests/test_cli_metadata.py | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/isic_cli/cli/__init__.py b/isic_cli/cli/__init__.py index 69c176d..ae6c65e 100644 --- a/isic_cli/cli/__init__.py +++ b/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,6 +161,7 @@ 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: diff --git a/tests/test_cli_metadata.py b/tests/test_cli_metadata.py index 4422187..c5cfbd9 100644 --- a/tests/test_cli_metadata.py +++ b/tests/test_cli_metadata.py @@ -4,6 +4,8 @@ import re import sys +from authlib.integrations.base_client.errors import OAuthError +from girder_cli_oauth_client import GirderCliOAuthClient import pytest from pytest_lazy_fixtures import lf @@ -82,6 +84,19 @@ def test_metadata_download_stdout(cli_runner): assert re.search(r"ISIC_0000000.*Foo.*CC-0.*melanoma.*male", result.output), result.output +@pytest.mark.usefixtures("_mock_image_metadata") +def test_metadata_download_stdout_failed_login_restore(cli_run, mocker): + mocker.patch.object( + GirderCliOAuthClient, "maybe_restore_login", side_effect=OAuthError(error="invalid_grant") + ) + + result = cli_run(["metadata", "download"]) + + assert result.exit_code == 0, result.exception + assert "Something went wrong with restoring a login" in result.stderr + assert result.stdout.startswith("isic_id,"), result.stdout + + @pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem") @pytest.mark.parametrize( "cli_runner", From 6726cdf33728bf9ede8042a9d4318697df61e324 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:36:23 -0400 Subject: [PATCH 07/11] Keep the API session open until the command finishes The session was opened with a with block in the cli group callback, so it was closed as soon as the callback returned, before any subcommand ran. Subcommands only worked because requests quietly creates new connection pools on a closed session. Register it on the root context instead, which closes it after the subcommand finishes. --- isic_cli/cli/__init__.py | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/isic_cli/cli/__init__.py b/isic_cli/cli/__init__.py index ae6c65e..dfbc19d 100644 --- a/isic_cli/cli/__init__.py +++ b/isic_cli/cli/__init__.py @@ -164,25 +164,26 @@ def cli(ctx, verbose: bool, guest: bool, sandbox: bool, dev: bool, no_version_ch 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") From 19ca7d21e14be557d4def484d433beec2c99100b Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:37:24 -0400 Subject: [PATCH 08/11] Stop when validating a search fails for a reason other than the query SearchString only checked for a 400 caused by an invalid query. Any other error, such as a 401 from an expired token or a 500, passed validation and the command went ahead as if the search were valid. --- isic_cli/cli/types.py | 1 + tests/test_cli_image.py | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/isic_cli/cli/types.py b/isic_cli/cli/types.py index 0891543..9bf12d1 100644 --- a/isic_cli/cli/types.py +++ b/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 diff --git a/tests/test_cli_image.py b/tests/test_cli_image.py index 0bad4c8..a5c03a6 100644 --- a/tests/test_cli_image.py +++ b/tests/test_cli_image.py @@ -6,7 +6,7 @@ import sys import pytest -from requests import HTTPError +from requests import HTTPError, Response from isic_cli.cli.image import cleanup_partially_downloaded_files @@ -140,6 +140,19 @@ def test_image_download_legacy_diagnosis_unsupported(cli_run, outdir): assert "no longer supported" in result.output +@pytest.mark.usefixtures("_isolated_filesystem", "_mock_images") +def test_image_download_search_server_error(mocker, cli_run, outdir): + response = Response() + response.status_code = 500 + mocker.patch("isic_cli.session.IsicCliSession.get", return_value=response) + + result = cli_run(["--guest", "image", "download", outdir, "--search", "age_approx:50"]) + + assert result.exit_code == 1, result.exception + assert "500 Server Error" in result.output + assert not Path(outdir).exists() + + @pytest.mark.skipif(sys.platform == "win32", reason="chmod doesn't restrict directory writes") @pytest.mark.usefixtures("_isolated_filesystem") def test_image_download_unwritable_outdir(cli_run): From 7de5749e35f537620a6ca7e97078a9051d86833d Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:38:31 -0400 Subject: [PATCH 09/11] Close the metadata download output file, and write it with no results The file passed to -o was opened without ever being closed, so writing it relied on the file object being garbage collected (and raised a ResourceWarning). When a search matched nothing, no output was written at all. With -o, the file wasn't created, since the writability check removes the file it probes. Write the CSV header regardless, so an empty result is still a valid CSV. --- isic_cli/cli/metadata.py | 5 +++-- tests/test_cli_metadata.py | 11 +++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/isic_cli/cli/metadata.py b/isic_cli/cli/metadata.py index 379aa88..bfdb456 100644 --- a/isic_cli/cli/metadata.py +++ b/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() diff --git a/tests/test_cli_metadata.py b/tests/test_cli_metadata.py index c5cfbd9..8b38cfc 100644 --- a/tests/test_cli_metadata.py +++ b/tests/test_cli_metadata.py @@ -113,6 +113,17 @@ def test_metadata_download_file(cli_runner): assert re.search(r"ISIC_0000000.*Foo.*CC-0.*melanoma.*male", output), output +@pytest.mark.usefixtures("_isolated_filesystem") +def test_metadata_download_no_results(cli_run, mocker): + mocker.patch("isic_cli.cli.metadata.get_num_images", return_value=0) + mocker.patch("isic_cli.cli.metadata.get_images", return_value=iter([])) + + result = cli_run(["metadata", "download", "-o", "foo.csv"]) + + assert result.exit_code == 0, result.exception + assert Path("foo.csv").read_text().splitlines() == ["isic_id,attribution,copyright_license"] + + @pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem") @pytest.mark.parametrize( "output_file", ["/metadata.csv", f"{'1' * 255}.csv"], ids=["no_permissions", "bad_filename"] From e4a52dfc21f08875576c1b948365b66b24c0ec80 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:39:07 -0400 Subject: [PATCH 10/11] Write license files as UTF-8 The license files were the only files image download wrote without an encoding, so they used the locale's encoding. On Windows that's usually cp1252, which can't encode every character a license might contain. --- isic_cli/cli/image.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/isic_cli/cli/image.py b/isic_cli/cli/image.py index 123c251..3879685 100644 --- a/isic_cli/cli/image.py +++ b/isic_cli/cli/image.py @@ -203,8 +203,8 @@ def download( 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") From d9890d0979c3eff4acf5ee7fe6e95df3747bfc90 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 14:39:49 -0400 Subject: [PATCH 11/11] Only suggest upgrading to a newer, non-yanked release upgrade_type compared the major, minor and micro parts separately without checking that the release was actually newer, so going from 2.0.0 to 1.5.0 counted as a minor upgrade. That can happen when running a version that isn't on PyPI yet. Yanked releases were also treated as available, so yanking a release would keep telling users to upgrade to it, or block them outright if it was a new major version. Read releases from PyPI's Index API (PEP 691) instead of the "releases" key of its JSON API, which PyPI has deprecated. If that key were removed, every command would crash, since the version check only handles request errors. The Index API lists files rather than releases, so versions are read from the file names, and a release with no files, which can't be installed, is no longer considered. --- isic_cli/utils/version.py | 27 +++++++++++++++++++++++---- tests/test_version.py | 29 +++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/isic_cli/utils/version.py b/isic_cli/utils/version.py index 65762fe..705321c 100644 --- a/isic_cli/utils/version.py +++ b/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] diff --git a/tests/test_version.py b/tests/test_version.py index 88409e3..f1dc963 100644 --- a/tests/test_version.py +++ b/tests/test_version.py @@ -3,17 +3,38 @@ from packaging.version import Version import pytest -from isic_cli.utils.version import newest_version_available +from isic_cli.utils.version import newest_version_available, upgrade_type @pytest.fixture() -def _mock_pypi_releases(mocker): +def _mock_pypi_files(mocker): mocker.patch( - "isic_cli.utils.version._pypi_releases", return_value={"0.0.1": None, "1.2.3": None} + "isic_cli.utils.version._pypi_files", + return_value=[ + {"filename": "isic_cli-0.0.1.tar.gz", "yanked": False}, + {"filename": "isic_cli-1.2.3-py3-none-any.whl"}, + {"filename": "isic_cli-1.2.3.tar.gz"}, + {"filename": "isic_cli-1.3.0-py3-none-any.whl", "yanked": True}, + {"filename": "isic_cli-1.4.0.tar.gz", "yanked": "Broken release"}, + ], ) -@pytest.mark.usefixtures("_mock_pypi_releases") +@pytest.mark.usefixtures("_mock_pypi_files") def test_newest_version_available(): newest_version = newest_version_available() assert newest_version == Version("1.2.3"), newest_version + + +@pytest.mark.parametrize( + ("from_version", "to_version", "expected"), + [ + ("1.2.3", "2.0.0", "major"), + ("1.2.3", "1.3.0", "minor"), + ("1.2.3", "1.2.4", "micro"), + ("1.2.3", "1.2.3", None), + ("2.0.0", "1.5.0", None), + ], +) +def test_upgrade_type(from_version, to_version, expected): + assert upgrade_type(Version(from_version), Version(to_version)) == expected