From b403fd47b4c51835cd255754e1c3bfb5d2741973 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Fri, 4 Sep 2026 13:59:09 +0200 Subject: [PATCH 1/2] Replace argh with cw, and fix the api-pkg-maker console script http2py depended on argh (LGPL) in three places. Move to cw (MIT, zero runtime deps). The three were not the same job. cli_maker.py -- MIGRATED, not deleted. It looks like scrap (a half-built ancestor of cw), but mk_cli and dispatch_cli are re-exported from http2py/__init__.py and tests/test_smoke.py asserts them as advertised public API. Deleting them would break `import http2py`. The care point there is the naming policy, and getting it wrong would have been silent. mk_cli called ArghParser().add_commands(...) with NO name_mapping_policy -- argh's LEGACY mode, which is not what argh.dispatch_commands does (that explicitly passes BY_NAME_IF_HAS_DEFAULT). Every function register_cli_method builds is all-keyword-only, and for a keyword-only parameter with no default the two modes disagree: argh legacy -> get-thing -u UID -p PID (required options) BY_NAME_IF_HAS_DEFAULT -> get-thing uid pid (positionals) cw.ARGH implements the latter, so plain cw.mk_parser would have changed the spelling of every required API argument with no error anywhere. CLI_CONVENTION pins cw.BY_NAME_IF_KWONLY, which reproduces the legacy grammar for this signature shape, and a test asserts the difference so the line cannot be dropped. dispatch_cli re-raises a non-zero code: cw.run RETURNS it where argh's parser.dispatch() exited. Success still returns None and still prints, exactly as before. api_pkg_maker.py -- FIXED, which is issue #14's open item 1. `api-pkg-maker` is this package's only console script and it could not start at all: `from setuptools import sandbox` raised ImportError at import time, before main() was ever entered, so the argh call below it had never been reached. sandbox.run_setup is now `setup.py sdist` in a subprocess -- which also removes the bare os.chdir(tempdir) that never chdir'd back. OUTPUT_DIR now uses expanduser rather than os.environ["HOME"], an import-time KeyError on Windows. The module is therefore importable again and collected by CI again, so its exclusion is removed from both pyproject.toml and conftest.py. The tripwire test that guarded "it is still broken" is inverted into one asserting it imports and that the console script's entry point exists -- which is what that tripwire was written to force. Verification: recorded argh's usage line for every command generated from a spec fixture exercising path args, typed query args, a request body with a required property, an apiKey scheme, and a no-argument route. Replayed under cw: zero diffs across all four surfaces. Proven load-bearing -- reverting the convention fails 3 tests. mk_api_pkg is covered end-to-end by a test that builds a real sdist offline and asserts the working directory survives. Closes #16. Advances #14 (item 1 answered: rewrite, not delete). Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr --- conftest.py | 17 ++- http2py/api_pkg_maker.py | 38 ++++-- http2py/cli_maker.py | 44 +++++-- http2py/example_cli.py | 13 +- pyproject.toml | 19 +-- tests/cli_goldens/mk_cli.json | 10 ++ tests/spec_fixture.py | 89 ++++++++++++++ tests/test_ci_collection_contract.py | 108 ++++++++++++++--- tests/test_cli_surface.py | 171 +++++++++++++++++++++++++++ 9 files changed, 448 insertions(+), 61 deletions(-) create mode 100644 tests/cli_goldens/mk_cli.json create mode 100644 tests/spec_fixture.py create mode 100644 tests/test_cli_surface.py diff --git a/conftest.py b/conftest.py index cf73e19..59c82f6 100644 --- a/conftest.py +++ b/conftest.py @@ -1,21 +1,18 @@ """Pytest configuration for the http2py repository. -``http2py.api_pkg_maker`` imports ``setuptools.sandbox``, which modern -setuptools no longer ships, so merely *importing* the module raises -``ImportError``. Under ``--doctest-modules`` that is not a single failing test: -it aborts collection for the whole session. The module is deliberately left in -place (whether to rewrite it or remove it is still open on issue #14), so it is -excluded from collection instead. +``http2py/tests`` stands up a real ``py2http`` web service, so it depends on a +test-only package that is not one of http2py's declared dependencies. Under +``--doctest-modules`` an unimportable module is not a single failing test: it +aborts collection for the whole session. So that directory is excluded here. -CI excludes the same two paths via ``[tool.wads.ci.testing].exclude_paths`` in +CI excludes the same path via ``[tool.wads.ci.testing].exclude_paths`` in pyproject.toml, which the wads ``run-tests-uv`` action turns into ``--ignore`` -flags. Repeating them here is what makes a bare ``pytest`` (no flags, e.g. a -local run or an editor's test runner) behave the same way as CI. +flags. Repeating it here is what makes a bare ``pytest`` (no flags, e.g. a local +run or an editor's test runner) behave the same way as CI. ``tests/test_ci_collection_contract.py`` asserts the two lists stay in agreement, so they cannot drift apart silently. """ collect_ignore = [ - "http2py/api_pkg_maker.py", "http2py/tests", ] diff --git a/http2py/api_pkg_maker.py b/http2py/api_pkg_maker.py index d68e899..21b04db 100644 --- a/http2py/api_pkg_maker.py +++ b/http2py/api_pkg_maker.py @@ -3,21 +3,26 @@ Given a spec (or the URL of one), :func:`mk_api_pkg` writes a small source distribution whose functions are bound to the service's routes. -Note: this module imports ``setuptools.sandbox``, which modern setuptools no -longer provides, so importing it raises ``ImportError``. It is kept in place -pending a decision (rewrite or remove) and is excluded from test collection; -see the repo-root ``conftest.py``. +The sdist is built by running ``setup.py sdist`` in a subprocess. This module +used to call ``setuptools.sandbox.run_setup``, which modern setuptools no longer +ships -- so merely *importing* it raised ``ImportError`` and the +``api-pkg-maker`` console script could not start at all (i2mint/http2py#14). +The subprocess is also better behaved: ``sandbox.run_setup`` needed the process +to ``os.chdir`` into the build directory and never came back. """ -import argh import os import shutil +import subprocess +import sys import tempfile from http2py.client import HttpClient from datetime import datetime, timezone -from setuptools import sandbox -OUTPUT_DIR = os.path.join(os.environ["HOME"], "http2py", "api_pkgs") +#: Where built packages are written. ``expanduser`` rather than +#: ``os.environ["HOME"]``: the latter is a module-import-time ``KeyError`` on +#: Windows, which would take the whole package down with it. +OUTPUT_DIR = os.path.join(os.path.expanduser("~"), "http2py", "api_pkgs") INIT_FILE_TPL = """from .funcs import {funcs} """ @@ -148,8 +153,12 @@ def create_file(filepath, content): filepath=os.path.join(tempdir, f"setup.cfg"), content=setup_cfg_content ) create_file(filepath=os.path.join(tempdir, f"setup.py"), content=SETUP_PY) - os.chdir(tempdir) - sandbox.run_setup("setup.py", ["sdist"]) + subprocess.run( + [sys.executable, "setup.py", "sdist"], + cwd=tempdir, + check=True, + capture_output=True, + ) pkg_filename = f"{pkg_name}-{pkg_version}.tar.gz" if not os.path.exists(OUTPUT_DIR): os.makedirs(OUTPUT_DIR) @@ -162,8 +171,15 @@ def create_file(filepath, content): def main(): - argh.dispatch_command(mk_api_pkg) + """Entry point for the ``api-pkg-maker`` console script; returns the exit code. + + ``cw.dispatch`` returns the code rather than exiting, and + ``[project.scripts]`` wraps this in ``sys.exit(main())``. + """ + import cw + + return cw.dispatch(mk_api_pkg) if __name__ == "__main__": - main() + raise SystemExit(main()) diff --git a/http2py/cli_maker.py b/http2py/cli_maker.py index de4c81a..15351b2 100644 --- a/http2py/cli_maker.py +++ b/http2py/cli_maker.py @@ -1,10 +1,14 @@ """Turn an http-bound python object into a command line interface. Signatures are first made argparse-friendly (:func:`mk_argparse_friendly`), -then dispatched with ``argh`` by :func:`mk_cli` / :func:`dispatch_cli`. +then turned into a parser by :func:`mk_cli` and run by :func:`dispatch_cli`. + +The parser is built by ``cw``. Its ``BY_NAME_IF_KWONLY`` naming is what +reproduces the grammar this module has always produced -- see +:data:`CLI_CONVENTION`. """ -import argh +import dataclasses from functools import wraps from glom import glom from inspect import signature @@ -15,11 +19,30 @@ from collections.abc import Callable, Iterable import yaml +import cw + from i2.io_trans import JSONAnnotAndDfltIoTrans from i2.signatures import set_signature_of_func, Sig, KO from http2py import HttpClient from http2py.authentication import mk_auth, DFLT_CONFIG_FILENAME +#: How the command line is derived from a signature. +#: +#: Every function :func:`register_cli_method` builds has KEYWORD-ONLY parameters +#: only (plus ``*args``/``**kwargs`` on a no-argument route), because +#: :func:`Sig.merge_with_sig` is called with ``kind=KO``. For that shape, +#: ``BY_NAME_IF_KWONLY`` is what reproduces the grammar this module produced +#: under argh: every parameter is an option, required exactly when it has no +#: default -- ``get-thing -u UID -p PID [-l LIMIT]``. +#: +#: cw's ``ARGH`` default (``BY_NAME_IF_HAS_DEFAULT``) would NOT: it turns a +#: keyword-only parameter with no default into a *positional*, so every required +#: API argument would silently change spelling. That is not a cw bug -- it +#: faithfully reproduces argh's *explicit* ``BY_NAME_IF_HAS_DEFAULT`` policy. +#: This module never selected a policy, and argh's no-policy legacy mode is the +#: one that matches ``BY_NAME_IF_KWONLY`` here. +CLI_CONVENTION = dataclasses.replace(cw.ARGH, naming=cw.BY_NAME_IF_KWONLY) + def mk_sig_argparse_friendly(sig): """Modifies a signature to change all leading underscores in param names @@ -109,15 +132,20 @@ def mk_cli( for methodname, method in client_details.__dict__.items() if getattr(method, "method_spec", None) ] - parser = argh.ArghParser() - parser.add_commands(cli_methods) - return parser + return cw.mk_parser(cli_methods, convention=CLI_CONVENTION) def dispatch_cli(*args, **kwargs): - """Makes a CLI parser with mk_cli and then dispatches it (see documentation of mk_cli)""" - parser = mk_cli(*args, **kwargs) - parser.dispatch() + """Makes a CLI parser with mk_cli and then runs it (see documentation of mk_cli) + + Returns ``None`` when the command ran, and raises ``SystemExit(2)`` on a + command-line error -- the same two outcomes the previous argh-based + implementation produced. ``cw.run`` *returns* the exit code rather than + exiting, so the non-zero case is re-raised here. + """ + code = cw.run(mk_cli(*args, **kwargs)) + if code: + raise SystemExit(code) set_signature_of_func(dispatch_cli, signature(mk_cli)) diff --git a/http2py/example_cli.py b/http2py/example_cli.py index 50a1403..fb36c7c 100644 --- a/http2py/example_cli.py +++ b/http2py/example_cli.py @@ -1,6 +1,6 @@ """A tiny worked example of the CLI-making tools, used by the docs and by hand.""" -import argh +import cw from collections.abc import Iterable from http2py.cli_maker import mk_argparse_friendly @@ -13,6 +13,11 @@ def myfun(a: int, b: Iterable[int]): if __name__ == "__main__": - parser = argh.ArghParser() - parser.add_commands([mk_argparse_friendly(AnnotAndDfltIoTrans()(myfun))]) - parser.dispatch() + from http2py.cli_maker import CLI_CONVENTION + + raise SystemExit( + cw.dispatch( + [mk_argparse_friendly(AnnotAndDfltIoTrans()(myfun))], + convention=CLI_CONVENTION, + ) + ) diff --git a/pyproject.toml b/pyproject.toml index 855e85a..1f1f484 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,7 +34,7 @@ dependencies = [ "i2", "ju", "requests", - "argh", + "cw>=0.1.1,<0.2", "PyYAML", "importlib_resources", # TEMPORARY, not imported by http2py: ju/oas.py has an unconditional (and @@ -160,15 +160,16 @@ coverage_threshold = 0 coverage_report_format = [ "term", ] -# `http2py.api_pkg_maker` imports `setuptools.sandbox`, which modern setuptools -# no longer ships, so the module raises ImportError on import and would abort -# the whole collection. Whether to rewrite it or drop it is still open on -# issue #14; excluding it from collection is the reversible middle course. -# The same two paths are repeated in the repo-root conftest.py so that a bare -# `pytest` behaves like CI -- tests/test_ci_collection_contract.py keeps the -# two lists in agreement. +# `http2py/tests` stands up a real py2http web service, so it needs a test-only +# dependency that CI does not install. Excluding it from collection is what keeps +# `pytest --doctest-modules` from aborting the session on the import. +# (`http2py/api_pkg_maker.py` used to be excluded here too, because +# `setuptools.sandbox` no longer exists and the module was unimportable. It is +# fixed and collected again -- see issue #14.) +# This path is repeated in the repo-root conftest.py so that a bare `pytest` +# behaves like CI -- tests/test_ci_collection_contract.py keeps the two lists +# in agreement. exclude_paths = [ - "http2py/api_pkg_maker.py", "http2py/tests", ] test_on_windows = false diff --git a/tests/cli_goldens/mk_cli.json b/tests/cli_goldens/mk_cli.json new file mode 100644 index 0000000..5636f57 --- /dev/null +++ b/tests/cli_goldens/mk_cli.json @@ -0,0 +1,10 @@ +{ + "recorded_from": "argh 0.31.3 via ArghParser().add_commands(...) with NO explicit name-mapping policy -- argh's legacy mode, which is what http2py.cli_maker.mk_cli used before the cw migration", + "note": "Normalised usage lines only: CPython rewrites --help bodies between versions and CI spans 3.10 and 3.12.", + "usages": { + "__top__": "usage: PROG [-h] {get-thing,make,plain} ...", + "get-thing": "usage: PROG get-thing [-h] -u UID -p PID [-v VERBOSE] [-l LIMIT] [-a API_KEY] [-c CONFIG]", + "make": "usage: PROG make [-h] -n NAME [--count COUNT] [-f FLAG] [-a API_KEY] [--config CONFIG]", + "plain": "usage: PROG plain [-h] [-a API_KEY] [-c CONFIG] [args ...]" + } +} \ No newline at end of file diff --git a/tests/spec_fixture.py b/tests/spec_fixture.py new file mode 100644 index 0000000..4fc3b75 --- /dev/null +++ b/tests/spec_fixture.py @@ -0,0 +1,89 @@ +"""One OpenAPI spec exercising every argument shape ``mk_cli`` has to handle. + +Path parameters (required, and typed), query parameters (optional, with and +without a schema default), a JSON request body with a ``required`` list, an +``apiKey`` security scheme (which adds an ``api_key`` CLI argument), and a route +with no arguments at all (which yields a ``*args``/``**kwargs`` signature). + +Kept as a module rather than inline so the golden recorder and the tests are +provably looking at the same spec. +""" + +CONTENT = "application/json" + +SPEC = { + "openapi": "3.0.2", + "info": {"title": "example", "version": "0.1"}, + "servers": [{"url": "https://example.com"}], + "security": {"apiKey": {}}, + "paths": { + "/u/{uid}/p/{pid}": { + "get": { + "x-method_name": "get_thing", + "description": "Two path args and two query args.", + "parameters": [ + { + "name": "uid", + "in": "path", + "required": True, + "schema": {"type": "string"}, + }, + { + "name": "pid", + "in": "path", + "required": True, + "schema": {"type": "integer"}, + }, + { + "name": "verbose", + "in": "query", + "required": False, + "schema": {"type": "boolean"}, + }, + { + "name": "limit", + "in": "query", + "required": False, + "schema": {"type": "integer", "default": 10}, + }, + ], + "responses": { + "200": {"description": "", "content": {CONTENT: {"schema": {}}}} + }, + } + }, + "/make": { + "post": { + "x-method_name": "make", + "description": "A request body with one required property.", + "requestBody": { + "content": { + CONTENT: { + "schema": { + "type": "object", + "properties": { + "name": {"type": "string"}, + "count": {"type": "integer"}, + "flag": {"type": "boolean"}, + }, + "required": ["name"], + } + } + } + }, + "responses": { + "200": {"description": "", "content": {CONTENT: {"schema": {}}}} + }, + } + }, + "/plain": { + "get": { + "x-method_name": "plain", + "description": "No arguments at all.", + "responses": { + "200": {"description": "", "content": {CONTENT: {"schema": {}}}} + }, + } + }, + }, +} diff --git a/tests/test_ci_collection_contract.py b/tests/test_ci_collection_contract.py index 105fd47..b50f36b 100644 --- a/tests/test_ci_collection_contract.py +++ b/tests/test_ci_collection_contract.py @@ -2,15 +2,16 @@ Under ``pytest --doctest-modules`` (what the CI test action runs) an unimportable module is not one red test, it is a collection abort that takes the whole session -down. Two modules in this repo are in that state: +down. -* ``http2py/api_pkg_maker.py`` imports ``setuptools.sandbox``, removed from - modern setuptools; -* ``http2py/tests/api_pkg_maker_test.py`` imports that module. +``http2py/api_pkg_maker.py`` used to be in that state -- it imported +``setuptools.sandbox``, removed from modern setuptools. It now builds the sdist +in a subprocess instead, imports cleanly, and is collected again. What remains +excluded is ``http2py/tests``, whose fixtures stand up a real ``py2http`` web +service: ``py2http`` is a test-only requirement that CI does not install. -Both are left in place on purpose (rewriting vs. deleting ``api_pkg_maker`` is -still open on issue #14) and excluded from collection in two places that must -agree: ``[tool.wads.ci.testing].exclude_paths`` in pyproject.toml, and +The exclusion lives in two places that must agree: +``[tool.wads.ci.testing].exclude_paths`` in pyproject.toml, and ``collect_ignore`` in the repo-root conftest.py. There is a third, subtler trap that these tests pin down: pytest eagerly imports @@ -31,10 +32,11 @@ REPO_ROOT = Path(__file__).resolve().parent.parent # Modules that are known-unimportable and therefore excluded from collection. -# Keep this in step with pyproject's exclude_paths and conftest's collect_ignore. -KNOWN_UNIMPORTABLE = {"http2py.api_pkg_maker"} +# Empty, and it should stay that way: an unimportable module aborts the whole +# CI session rather than failing one test. +KNOWN_UNIMPORTABLE: set = set() -EXPECTED_EXCLUDED_PATHS = {"http2py/api_pkg_maker.py", "http2py/tests"} +EXPECTED_EXCLUDED_PATHS = {"http2py/tests"} def _package_modules(): @@ -62,17 +64,27 @@ def test_every_package_module_imports_except_the_known_broken_one(): assert not failures, f"modules that would abort collection: {failures}" -def test_known_broken_module_is_still_broken(): - """Tripwire for issue #14. +def test_api_pkg_maker_imports_and_the_console_script_can_start(): + """The former tripwire for issue #14, inverted now that the module is fixed. - If ``api_pkg_maker`` starts importing again (someone rewrote it, or removed - it), the exclusions below are dead weight and the #14 (a)/(b) question is - answered -- so this deliberately fails to force that cleanup rather than - letting a stale exclusion sit there forever. + ``api-pkg-maker`` is this package's only console script, and it could not + start at all: ``from setuptools import sandbox`` raised ``ImportError`` at + import time, before ``main()`` was ever entered. Both halves are asserted -- + the module imports, and the entry point the console script calls is there. """ - for name in KNOWN_UNIMPORTABLE: - with pytest.raises(ImportError): - importlib.import_module(name) + module = importlib.import_module("http2py.api_pkg_maker") + assert callable(module.main) + assert callable(module.mk_api_pkg) + + +def test_no_module_is_excluded_for_being_unimportable(): + """Guard the invariant, not the past exception. + + ``KNOWN_UNIMPORTABLE`` is empty. If a future change adds a module that + cannot be imported, the honest fix is to fix the module -- not to grow this + set -- because an unimportable module aborts collection for everything. + """ + assert KNOWN_UNIMPORTABLE == set() @pytest.mark.skipif(sys.version_info < (3, 11), reason="tomllib needs Python 3.11+") @@ -137,3 +149,61 @@ def find_spec(self, fullname, path=None, target=None): sys.modules.pop("_http2py_tests_conftest", None) assert hasattr(module, "ws_app") + + +def test_mk_api_pkg_builds_a_source_distribution(tmp_path, monkeypatch): + """End-to-end proof that the ``setuptools.sandbox`` fix actually works. + + Builds a real sdist from a spec dict -- no network, no live service -- and + checks the archive is where ``mk_api_pkg`` says it is. Also asserts the + working directory survives: the old implementation did a bare + ``os.chdir(tempdir)`` and never came back, which quietly broke any caller + that used relative paths afterwards. + """ + import tarfile + + from http2py import api_pkg_maker + + monkeypatch.setattr(api_pkg_maker, "OUTPUT_DIR", str(tmp_path)) + cwd_before = Path.cwd() + + spec = { + "openapi": "3.0.2", + "info": {"title": "d", "version": "0.1"}, + "servers": [{"url": "http://localhost:3030"}], + "paths": { + "/foo": { + "post": { + "x-method_name": "foo", + "description": "foo.", + "requestBody": { + "required": True, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": {"a": {"type": "integer"}}, + } + } + }, + }, + "responses": { + "200": { + "description": "", + "content": {"application/json": {"schema": {}}}, + } + }, + } + } + }, + } + + built = api_pkg_maker.mk_api_pkg(spec, pkg_name="probepkg", pkg_version="1.2.3") + + assert Path(built).is_file() + assert Path(built).name == "probepkg-1.2.3.tar.gz" + assert Path(built).parent == tmp_path + assert Path.cwd() == cwd_before, "mk_api_pkg left the process in another directory" + with tarfile.open(built) as archive: + names = {n.split("/", 1)[-1] for n in archive.getnames()} + assert "probepkg/funcs.py" in names diff --git a/tests/test_cli_surface.py b/tests/test_cli_surface.py new file mode 100644 index 0000000..f10c1b8 --- /dev/null +++ b/tests/test_cli_surface.py @@ -0,0 +1,171 @@ +"""Pin ``mk_cli``'s generated command line against what argh produced. + +``http2py.cli_maker`` builds a CLI out of an OpenAPI spec. The golden in +``tests/cli_goldens/mk_cli.json`` records the ``usage:`` line argh produced for +every command of ``tests/spec_fixture.SPEC``, recorded through +``ArghParser().add_commands(...)`` -- exactly the call ``mk_cli`` used to make -- +before the migration to ``cw``. + +Only usage lines are asserted, not full ``--help`` bodies: CPython rewrites the +option column between versions and CI spans 3.10 and 3.12. The full-body diff +(zero differences across all four surfaces) was done at migration time and is +recorded in the pull request. +""" + +from __future__ import annotations + +import dataclasses +import json +import sys +from pathlib import Path + +import pytest + +import cw + +sys.path.insert(0, str(Path(__file__).parent)) + +from spec_fixture import SPEC # noqa: E402 + +GOLDEN = json.loads( + (Path(__file__).parent / "cli_goldens" / "mk_cli.json").read_text(encoding="utf-8") +) +EXPECTED = GOLDEN["usages"] + + +def _usages(parser): + """``{command_name: normalised usage line}``, plus ``__top__`` for the parser.""" + out = {"__top__": " ".join(parser.format_usage().split())} + for action in parser._actions: + if getattr(action, "choices", None): + for name, sub in action.choices.items(): + out[name] = " ".join(sub.format_usage().split()) + return out + + +def _parser(**kwargs): + from http2py.cli_maker import mk_cli + + return mk_cli(openapi_spec=SPEC, **kwargs) + + +def test_mk_cli_returns_a_parser_with_one_command_per_route(): + parser = _parser() + assert set(_usages(parser)) == {"__top__", "get-thing", "make", "plain"} + + +@pytest.mark.parametrize("command", sorted(EXPECTED)) +def test_usage_matches_the_argh_recording(command): + """Every command's grammar is byte-identical to what argh generated. + + The recorded golden used ``prog="PROG"``; a parser built during a test run + derives its prog from ``sys.argv[0]``. Only what follows the prog is + compared, so both sides drop ``usage:`` plus the prog tokens. + """ + actual = _usages(_parser()) + offset = 1 if command == "__top__" else 2 # subparser progs are two tokens + assert actual[command].split()[1 + offset :] == EXPECTED[command].split()[ + 1 + offset : + ], f"{command}: {actual[command]!r} != {EXPECTED[command]!r}" + + +def test_required_api_arguments_are_options_not_positionals(): + """The load-bearing consequence of ``CLI_CONVENTION``. + + Every function ``register_cli_method`` builds is all-keyword-only, so a + required argument like ``uid`` must be spelled ``-u UID`` / ``--uid UID``. + Under cw's ARGH default it would become a bare positional and every caller's + command line would silently change. + """ + usage = _usages(_parser())["get-thing"] + assert "-u UID" in usage and "-p PID" in usage + assert " uid" not in usage and " pid" not in usage + + +def test_the_convention_is_load_bearing_and_this_test_can_fail(): + """Prove the check above would catch losing ``naming=BY_NAME_IF_KWONLY``. + + If this stops holding, cw's default naming has become equivalent for this + signature shape and ``CLI_CONVENTION``'s comment is stale -- not the other + way round. + """ + from http2py.cli_maker import CLI_CONVENTION, register_cli_method + from http2py.client import HttpClient + + client = HttpClient(SPEC) + methods = [ + register_cli_method(SPEC, m, ["api_key"]) + for _, m in client.__dict__.items() + if getattr(m, "method_spec", None) + ] + default = cw.mk_parser( + methods, + convention=dataclasses.replace(CLI_CONVENTION, naming=cw.ARGH.naming), + prog="PROG", + ) + usage = _usages(default)["get-thing"] + assert "-u UID" not in usage, ( + "cw's default naming was expected to turn the required keyword-only " + f"arguments into positionals, but produced {usage!r}" + ) + + +def test_every_parameter_register_cli_method_builds_is_keyword_only(): + """The premise ``CLI_CONVENTION`` rests on, asserted rather than assumed. + + ``BY_NAME_IF_KWONLY`` reproduces argh's legacy grammar *because* these + signatures are all keyword-only. If a future change introduced an ordinary + positional parameter, the two policies would diverge for it and the golden + above is what would catch it -- but this says so directly. + """ + import inspect + + from http2py.cli_maker import register_cli_method + from http2py.client import HttpClient + + client = HttpClient(SPEC) + for _, method in client.__dict__.items(): + if not getattr(method, "method_spec", None): + continue + func = register_cli_method(SPEC, method, ["api_key"]) + for param in inspect.signature(func).parameters.values(): + assert param.kind in ( + param.KEYWORD_ONLY, + param.VAR_POSITIONAL, + param.VAR_KEYWORD, + ), f"{func.__name__}.{param.name} is {param.kind}" + + +def test_dispatch_cli_raises_SystemExit_on_a_command_line_error(): + """``cw.run`` RETURNS the exit code; ``dispatch_cli`` must still raise it. + + argh's ``parser.dispatch()`` exited the process on a bad command line. + Without the re-raise, a broken invocation would look like success. + """ + from http2py.cli_maker import dispatch_cli + + argv = sys.argv + sys.argv = ["PROG", "no-such-command"] + try: + with pytest.raises(SystemExit) as excinfo: + dispatch_cli(openapi_spec=SPEC) + assert excinfo.value.code == 2 + finally: + sys.argv = argv + + +def test_the_package_no_longer_imports_argh(): + """http2py no longer declares argh; nothing may import it.""" + import subprocess + + proc = subprocess.run( + [ + sys.executable, + "-c", + "import sys, http2py, http2py.cli_maker, http2py.api_pkg_maker; " + "sys.exit(1 if 'argh' in sys.modules else 0)", + ], + capture_output=True, + text=True, + ) + assert proc.returncode == 0, f"argh was imported: {proc.stderr}" From 4e2276dfd32c79d45caa8b60e97f47f94e9fa015 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:03:18 +0200 Subject: [PATCH 2/2] Make mk_api_pkg's setuptools requirement explicit and legible CI caught a real gap in the previous commit: uv-created virtualenvs do not ship setuptools, so `setup.py sdist` exited 1 and `check=True` reported only the exit status -- no cause. The old implementation hid this: `from setuptools import sandbox` made setuptools an undeclared IMPORT-time requirement of the module. Three changes, all about making the requirement visible rather than adding it to the package: - _check_build_backend() raises an actionable RuntimeError naming setuptools and the exact pip command, before any temp directory is created. - A failed `setup.py sdist` now raises with the captured stdout and stderr. check=True discarded exactly the output that says what went wrong. - setuptools added to the `dev` extra, NOT to the runtime dependencies. It is needed by this one function, not by `import http2py`, and CI installs `-e ".[dev]"` -- so the end-to-end test really runs there rather than skipping. The test also gained an importorskip so a bare environment skips it cleanly instead of failing for the wrong reason. Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr --- http2py/api_pkg_maker.py | 38 ++++++++++++++++++++++++++-- pyproject.toml | 5 ++++ tests/test_ci_collection_contract.py | 6 +++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/http2py/api_pkg_maker.py b/http2py/api_pkg_maker.py index 21b04db..a61f26c 100644 --- a/http2py/api_pkg_maker.py +++ b/http2py/api_pkg_maker.py @@ -9,6 +9,12 @@ ``api-pkg-maker`` console script could not start at all (i2mint/http2py#14). The subprocess is also better behaved: ``sandbox.run_setup`` needed the process to ``os.chdir`` into the build directory and never came back. + +``setuptools`` is still needed to *run* :func:`mk_api_pkg`, but it is +deliberately NOT a declared dependency of http2py: it is needed by this one +function, not by ``import http2py``, and a virtualenv created by ``uv`` does not +ship it. :func:`_check_build_backend` turns its absence into an actionable +message instead of a subprocess exit code. """ import os @@ -116,6 +122,8 @@ def create_file(filepath, content): with open(filepath, "w") as file: print(content, file=file) + _check_build_backend() + tempdir = tempfile.mkdtemp() try: api = HttpClient(openapi_spec=openapi_spec, url=openapi_url) @@ -153,12 +161,20 @@ def create_file(filepath, content): filepath=os.path.join(tempdir, f"setup.cfg"), content=setup_cfg_content ) create_file(filepath=os.path.join(tempdir, f"setup.py"), content=SETUP_PY) - subprocess.run( + build = subprocess.run( [sys.executable, "setup.py", "sdist"], cwd=tempdir, - check=True, capture_output=True, + text=True, ) + if build.returncode != 0: + # check=True would raise a CalledProcessError carrying only the exit + # status, discarding the captured output that says what went wrong. + raise RuntimeError( + f"`setup.py sdist` failed (exit {build.returncode}) while building " + f"{pkg_name!r}.\n--- stdout ---\n{build.stdout}" + f"\n--- stderr ---\n{build.stderr}" + ) pkg_filename = f"{pkg_name}-{pkg_version}.tar.gz" if not os.path.exists(OUTPUT_DIR): os.makedirs(OUTPUT_DIR) @@ -170,6 +186,24 @@ def create_file(filepath, content): shutil.rmtree(tempdir) +def _check_build_backend(): + """Fail early and legibly when the interpreter cannot build an sdist. + + ``setup.py sdist`` needs setuptools in the *subprocess's* interpreter, which + is this one. Environments created by ``uv`` (and ``python -m venv + --without-pip``) do not have it. Without this check the user sees a bare + ``CalledProcessError`` with an exit status and no cause. + """ + import importlib.util + + if importlib.util.find_spec("setuptools") is None: + raise RuntimeError( + "mk_api_pkg builds a source distribution with `setup.py sdist`, " + f"which needs setuptools in {sys.executable}. Install it with: " + f"{sys.executable} -m pip install setuptools" + ) + + def main(): """Entry point for the ``api-pkg-maker`` console script; returns the exit code. diff --git a/pyproject.toml b/pyproject.toml index 1f1f484..63f63f3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,6 +57,11 @@ dev = [ "pytest>=7.0", "pytest-cov>=4.0", "ruff>=0.1.0", + # Not a runtime dependency of the package: `mk_api_pkg` shells out to + # `setup.py sdist`, so setuptools is needed to exercise it. A uv-created + # venv does not ship setuptools, so without this the end-to-end + # api_pkg_maker test would silently skip in CI. + "setuptools", ] [tool.hatch.build.targets.wheel] diff --git a/tests/test_ci_collection_contract.py b/tests/test_ci_collection_contract.py index b50f36b..1094ac4 100644 --- a/tests/test_ci_collection_contract.py +++ b/tests/test_ci_collection_contract.py @@ -162,6 +162,12 @@ def test_mk_api_pkg_builds_a_source_distribution(tmp_path, monkeypatch): """ import tarfile + pytest.importorskip( + "setuptools", + reason="mk_api_pkg shells out to `setup.py sdist`; setuptools is in the " + "dev extra, so this runs in CI but skips in a bare environment", + ) + from http2py import api_pkg_maker monkeypatch.setattr(api_pkg_maker, "OUTPUT_DIR", str(tmp_path))