Skip to content

Fix the help-text leak and correct the documented error mapping#16

Merged
maltemindedal merged 1 commit into
mainfrom
fix/code-review-findings
Jul 24, 2026
Merged

Fix the help-text leak and correct the documented error mapping#16
maltemindedal merged 1 commit into
mainfrom
fix/code-review-findings

Conversation

@maltemindedal

Copy link
Copy Markdown
Owner

A two-axis review of the codebase — against the repo's documented standards, and against its own documentation treated as the spec — turned up eighteen findings. This addresses all of them.

Behaviour changes are limited to the CLI help text. Everything else either makes the documentation match the code or removes machinery nothing used.

Fixed

  • The -d/--data help printed a literal R| prefix. The marker belonged to a custom formatter attached only to the top-level parser — never to the subcommands that actually carried the marker — so it leaked into the very output it was meant to control. Formatter and marker are both gone.
  • The documented exception mapping was wrong twice over. Retries exhausted against a transient status raise ResponseError carrying the real status, not HTTPConnectionError: the adapter sets raise_on_status=False, so the last response is reported rather than discarded. And a connect timeout is an HTTPConnectionError, since requests.exceptions.ConnectTimeout subclasses ConnectionError; only read timeouts reach HTTPClientError. Corrected in the API reference and in both guides that repeated the claim.
  • allowed_methods did nothing. It was assigned in __init__ and documented as an instance attribute, but every request validated against the ALLOWED_METHODS class constant instead. It is now the attribute method validation reads.

Changed

  • __version__ is read from the installed distribution's metadata instead of being a second hand-maintained copy of the version in pyproject.toml. It resolves through the existing PEP 562 hook, so import snaffle does not pay for the lookup; ADR 0002 records the new deferred import.
  • The CLI dispatches through make_request(method, url) rather than looking the per-verb method up by name with getattr, which had erased the typed API. _emit_response is annotated requests.Response via a TYPE_CHECKING import, keeping the help path clear of the HTTP stack.

Removed

  • cli.show_examples and main's suppress_output parameter. Neither is part of the documented public API — that is HTTPClient and the three exceptions — and the flag existed only to quiet tests that already redirect stdout. Filed under Removed in the changelog rather than as a breaking change for that reason; happy to keep the parameter if you'd rather.

Tests

40 → 46.

  • Dropped a retry test that mocked requests.Session.request. That mock sits above the adapter and cannot observe retries at all, so the assertion was vacuous — it would have passed with 404 in RETRY_STATUSES. This is exactly the trap CONTRIBUTING.md documents, and TestRetryAgainstRealServer already covers the case correctly.
  • Split the duplicated large/small-file progress tests so each patches tqdm and asserts whether a bar was drawn. Both previously made the identical assertion, and the small-file case never checked the suppression it was named for.
  • Added tests/test_init.py and tests/test_main.py, the two source modules that had no test module despite the one-per-source-module convention. They cover the Ctrl+C-exits-0 contract of the console script, lazy resolution of HTTPClient and __version__, and a subprocess guard that a bare import snaffle does not pull in requests.

Three judgement calls

Three findings were doc-vs-code conflicts. I fixed the docs, not the behaviour:

Conflict Why the code wins
Exhausted status retries raise_on_status=False is a deliberate, separately documented choice, and a ResponseError carrying the real 503 tells the caller more than a generic connection failure. The existing real-socket test already asserted it.
Connect timeouts A timeout while the connection is still being established is a connection failure.
[VERBOSE] error lines Printing the underlying requests exception is what verbose mode is for. Documented rather than removed.

Reversing any of these is a behavioural break, so flag it if you'd prefer the other direction.

Verification

All four checks from CONTRIBUTING.md pass locally on 3.14:

uv run ruff check .            All checks passed!
uv run ruff format --check .   10 files already formatted
uv run mypy .                  Success: no issues found in 10 source files
uv run python -m unittest discover tests   Ran 46 tests — OK

Smoke-tested end to end against a live host: GET renders, a 404 exits 1 with the status in the message, -v prints the underlying exception, python -m snaffle prints help, and snaffle.__version__ resolves to 3.0.0 from metadata — unchanged from the hard-coded value.

🤖 Generated with Claude Code

A two-axis review of the codebase -- against the repo's own standards and
against its documentation as a spec -- turned up eighteen findings. Behaviour
changes are limited to the CLI help text; everything else either makes the
documentation match the code or removes machinery nothing used.

Fixed:

- The `-d/--data` help printed a literal `R|` prefix. The marker belonged to a
  custom formatter attached only to the top-level parser, never to the
  subcommands that carried the marker, so it leaked into the output it was
  meant to control. Formatter and marker are both gone.
- The documented exception mapping was wrong twice over. Retries exhausted
  against a transient status raise `ResponseError` with the real status, not
  `HTTPConnectionError`: the adapter sets `raise_on_status=False`, so the last
  response is reported rather than discarded. And a connect timeout is an
  `HTTPConnectionError`, since `ConnectTimeout` subclasses `ConnectionError`;
  only read timeouts reach `HTTPClientError`. The docs now say what the code
  does. Corrected in the API reference and in both guides that repeated it.
- `allowed_methods` was assigned and documented but never read -- validation
  went to the `ALLOWED_METHODS` class constant. It is now the attribute every
  request checks.

Changed:

- `__version__` is read from the installed distribution's metadata instead of
  being a second hand-maintained copy of the version in `pyproject.toml`. It
  resolves through the existing PEP 562 hook, so `import snaffle` does not pay
  for the lookup; ADR 0002 records the new deferred import.
- The CLI dispatches through `make_request(method, url)` rather than looking
  the per-verb method up by name with `getattr`, which had erased the typed
  API. `_emit_response` is annotated `requests.Response` via a `TYPE_CHECKING`
  import, which keeps the help path clear of the HTTP stack.

Removed:

- `cli.show_examples` and `main`'s `suppress_output` parameter. Neither is part
  of the documented public API, and the flag existed only to quiet tests that
  already redirect stdout.

Tests:

- Dropped a retry test that mocked `Session.request`. That mock sits above the
  adapter and cannot observe retries, so the assertion was vacuous -- exactly
  the trap CONTRIBUTING documents. The real-socket test already covers it.
- Split the duplicated large/small-file progress tests so each patches `tqdm`
  and asserts whether a bar was drawn. The small-file case never checked the
  suppression it was named for.
- Added `test_init.py` and `test_main.py`, the two source modules that had no
  test module. They cover the Ctrl+C-exits-zero contract of the console script,
  lazy resolution of `HTTPClient` and `__version__`, and a subprocess guard
  that a bare `import snaffle` does not pull in `requests`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@maltemindedal
maltemindedal merged commit 969ba71 into main Jul 24, 2026
8 of 9 checks passed
@maltemindedal
maltemindedal deleted the fix/code-review-findings branch July 24, 2026 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant