From a67cc0310f6d3cfdaed4979ad9937f6d240f7720 Mon Sep 17 00:00:00 2001 From: Justin Date: Sat, 11 Apr 2026 23:07:48 -0500 Subject: [PATCH] chore(simplify): trim comment noise + remove dead state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three parallel review agents (code reuse, quality, efficiency) reviewed the full PR #8 diff. Reuse and efficiency: clean. Quality found actionable comment density and one dead-state issue. Fixed: - Remove dead `_user_agent` attribute from both clients — stored in __init__ but never read after being baked into `_base_headers`. Inlined the `user_agent or default_user_agent()` expression directly into the headers dict. - Trim `_raw_request` docstring from 31 lines to 7 in both clients — the hypothetical scenarios and precedence-order details are over-documentation for a private method with one non-default caller. - Trim `_validate_batch_create_shape._fail` comments from 14 lines to 2 — was more comment than code. - Trim `parse_quota` plan-default comment from 6 lines to 1. - Trim `build_list_params` type-annotation comment (3 lines → 0). - Trim spec-validation section banner (3 lines → 1). - Remove `cast` explanation comments from both clients (4 lines each). Skipped (not worth the churn): - Sync/async duplication — tracked as #20, structural limitation. - Parameter sprawl on list()/list_all() — at threshold but not over. - HTTP method Literal type annotation — low risk for a private method. - Double getattr in _serialize_value — negligible cost on max 7 fields. Net: -78 lines across 3 files. 158 tests passing (unchanged). --- src/layerv_qurl/_utils.py | 28 ++++------------------ src/layerv_qurl/async_client.py | 41 +++++---------------------------- src/layerv_qurl/client.py | 41 +++++---------------------------- 3 files changed, 16 insertions(+), 94 deletions(-) diff --git a/src/layerv_qurl/_utils.py b/src/layerv_qurl/_utils.py index 9c840e3..0010957 100644 --- a/src/layerv_qurl/_utils.py +++ b/src/layerv_qurl/_utils.py @@ -134,10 +134,7 @@ def build_body(kwargs: dict[str, Any]) -> dict[str, Any]: return body -# ---- Spec-derived input validation -------------------------------------- -# These mirror the constraints documented on each request schema in -# qurl/api/openapi.yaml so obvious mistakes fail fast with a ValueError -# instead of round-tripping to the API and coming back as a generic 400. +# ---- Spec-derived input validation (mirrors openapi.yaml constraints) ---- MAX_TARGET_URL = 2048 MAX_LABEL = 500 @@ -410,13 +407,7 @@ def parse_quota(data: dict[str, Any]) -> Quota: total_accesses=usage_data.get("total_accesses", 0), ) return Quota( - # Fall back to the same sentinel the dataclass default uses - # (see ``Quota.plan`` in types.py) so a malformed API response - # that omits the field produces a consistent "not-yet-populated" - # value regardless of whether the Quota was constructed via - # parse_quota or directly. In practice the /v1/quota endpoint - # always returns a populated plan string, so this fallback is - # only hit for malformed responses or internal bootstrap paths. + # Match the Quota dataclass default for consistency on malformed responses. plan=data.get("plan", "unknown"), period_start=_parse_dt(data.get("period_start")), period_end=_parse_dt(data.get("period_end")), @@ -457,22 +448,14 @@ def _validate_batch_create_shape(data: Any) -> None: """ def _fail(reason: str, *, top_level_keys: list[str] | None = None) -> ValidationError: - # DEBUG log carries structural hints (type + top-level key names - # only — JSON keys come from the published schema, not user - # data) so operators can triage shape-guard trips without - # leaking raw body content into logs. logger.debug( "batch_create shape guard tripped: %s (type=%s, top_level_keys=%s)", reason, type(data).__name__, top_level_keys, ) - # Uses `ValidationError` (subclass) not bare `QURLError` so - # `except ValidationError` catches shape-guard trips; `code= - # "unexpected_response"` distinguishes from client-side - # preflight (`client_validation`). `status=0` is the SDK - # convention for all client-detected failures (not real HTTP - # status). See qurl-typescript's `unexpectedResponseError`. + # ValidationError with code="unexpected_response" (not "client_validation") + # so callers can distinguish server shape mismatches from preflight errors. return ValidationError( status=0, code="unexpected_response", @@ -671,9 +654,6 @@ def build_list_params( raise ValueError( f"limit: must be an integer between 1 and 100 (got {limit})" ) - # ``status`` is a QURLStatus (Literal | str) — covered by the ``str`` arm. - # Ordered most-specific to least-specific: datetime/int are concrete - # types, str is the widening arm, None is the "drop" sentinel. pairs: dict[str, datetime | int | str | None] = { "limit": limit, "cursor": cursor, diff --git a/src/layerv_qurl/async_client.py b/src/layerv_qurl/async_client.py index 5b23502..06715d5 100644 --- a/src/layerv_qurl/async_client.py +++ b/src/layerv_qurl/async_client.py @@ -95,13 +95,12 @@ def __init__( self._base_url = base_url.rstrip("/") self._api_key = api_key self._max_retries = max_retries - self._user_agent = user_agent or default_user_agent() self._client = http_client or httpx.AsyncClient(timeout=timeout) self._owns_client = http_client is None self._base_headers: dict[str, str] = { "Authorization": f"Bearer {api_key}", "Accept": "application/json", - "User-Agent": self._user_agent, + "User-Agent": user_agent or default_user_agent(), } def __repr__(self) -> str: @@ -517,10 +516,6 @@ async def batch_create( ) from exc except ValueError as exc: raise ValueError(f"batch_create items[{i}]: {exc}") from exc - # `BatchCreateItem` is structurally a `dict[str, Any]` at runtime — - # TypedDicts compile to plain dicts and carry no runtime overhead. - # The `cast` narrows the type for `build_body` without any runtime - # conversion. serialized = [build_body(cast("dict[str, Any]", item)) for item in items] # HTTP 400 carries structured per-item errors on this endpoint — # whitelist it so the generic error path doesn't swallow the body. @@ -589,35 +584,11 @@ async def _raw_request( ) -> tuple[Any, dict[str, Any] | None]: """Issue an HTTP request and parse the JSON envelope. - ``allow_statuses`` lets a caller opt specific non-2xx codes out of - the default raise-on-error path and receive the parsed body - instead. This is used by :meth:`batch_create`, where the API - returns a structured ``BatchCreateOutput`` on HTTP 400 (all items - rejected) — raising would drop the per-item errors. - - **`allow_statuses` takes precedence over retries.** The check - order in the response-handling loop is: - - 1. ``response.status_code < 400 or in allow_statuses`` → - return the parsed body immediately as a success. - 2. Otherwise, build an error and check the retry filter - (``RETRYABLE_STATUS_POST`` for POST, ``RETRYABLE_STATUS`` - for everything else). - - This means a status listed in ``allow_statuses`` is returned - to the caller **without ever running through the retry - filter**, even if that status would normally be retried. For - the only current use case (``batch_create`` with - ``allow_statuses=(400,)``) the interaction is harmless because - 400 isn't in any retry set — a 400 carries the authoritative - per-item errors and retrying would just reproduce them. - - Callers adding a *retryable* status (e.g. 429 or 5xx) to - ``allow_statuses`` should be aware this bypasses the SDK's - retry path entirely: the status is surfaced on the first - attempt with no transparent backoff. If that's not what you - want, leave the status out of ``allow_statuses`` and let the - normal retry logic handle it. + ``allow_statuses`` opts specific non-2xx codes out of the + raise-on-error path, returning the parsed body instead. Used by + :meth:`batch_create` (``allow_statuses=(400,)``) so per-item + errors aren't swallowed. Allowed statuses bypass the retry + filter entirely — they're returned on the first attempt. """ url = f"{self._base_url}{path}" last_error: Exception | None = None diff --git a/src/layerv_qurl/client.py b/src/layerv_qurl/client.py index cd8a885..3483e29 100644 --- a/src/layerv_qurl/client.py +++ b/src/layerv_qurl/client.py @@ -110,13 +110,12 @@ def __init__( self._base_url = base_url.rstrip("/") self._api_key = api_key self._max_retries = max_retries - self._user_agent = user_agent or default_user_agent() self._client = http_client or httpx.Client(timeout=timeout) self._owns_client = http_client is None self._base_headers: dict[str, str] = { "Authorization": f"Bearer {api_key}", "Accept": "application/json", - "User-Agent": self._user_agent, + "User-Agent": user_agent or default_user_agent(), } def __repr__(self) -> str: @@ -531,10 +530,6 @@ def batch_create( ) from exc except ValueError as exc: raise ValueError(f"batch_create items[{i}]: {exc}") from exc - # `BatchCreateItem` is structurally a `dict[str, Any]` at runtime — - # TypedDicts compile to plain dicts and carry no runtime overhead. - # The `cast` narrows the type for `build_body` without any runtime - # conversion. serialized = [build_body(cast("dict[str, Any]", item)) for item in items] # HTTP 400 carries structured per-item errors on this endpoint — # whitelist it so the generic error path doesn't swallow the body. @@ -603,35 +598,11 @@ def _raw_request( ) -> tuple[Any, dict[str, Any] | None]: """Issue an HTTP request and parse the JSON envelope. - ``allow_statuses`` lets a caller opt specific non-2xx codes out of - the default raise-on-error path and receive the parsed body - instead. This is used by :meth:`batch_create`, where the API - returns a structured ``BatchCreateOutput`` on HTTP 400 (all items - rejected) — raising would drop the per-item errors. - - **`allow_statuses` takes precedence over retries.** The check - order in the response-handling loop is: - - 1. ``response.status_code < 400 or in allow_statuses`` → - return the parsed body immediately as a success. - 2. Otherwise, build an error and check the retry filter - (``RETRYABLE_STATUS_POST`` for POST, ``RETRYABLE_STATUS`` - for everything else). - - This means a status listed in ``allow_statuses`` is returned - to the caller **without ever running through the retry - filter**, even if that status would normally be retried. For - the only current use case (``batch_create`` with - ``allow_statuses=(400,)``) the interaction is harmless because - 400 isn't in any retry set — a 400 carries the authoritative - per-item errors and retrying would just reproduce them. - - Callers adding a *retryable* status (e.g. 429 or 5xx) to - ``allow_statuses`` should be aware this bypasses the SDK's - retry path entirely: the status is surfaced on the first - attempt with no transparent backoff. If that's not what you - want, leave the status out of ``allow_statuses`` and let the - normal retry logic handle it. + ``allow_statuses`` opts specific non-2xx codes out of the + raise-on-error path, returning the parsed body instead. Used by + :meth:`batch_create` (``allow_statuses=(400,)``) so per-item + errors aren't swallowed. Allowed statuses bypass the retry + filter entirely — they're returned on the first attempt. """ url = f"{self._base_url}{path}" last_error: Exception | None = None