From 4c67145ed566b139573bca19013688e589a99d7e Mon Sep 17 00:00:00 2001 From: Andrew Miller Date: Wed, 17 Jun 2026 12:08:49 -0500 Subject: [PATCH 1/4] Address top backlog consistency fixes - Convert FacetSort from Literal TypeAlias to str Enum matching SortOrder pattern; update FacetsFilter.sort default to FacetSort.COUNT_DESC and serialize with .value. - Export FacetSort from visor package root alongside FacetsFilter. - Add tests for FacetSort default, explicit enum, raw-string coercion, and wire values. - Add include-field semantics tests (None/[]/[...]) for ListingDetail.price_history, ListingSnapshot.price_history, and VehicleBuild.options. - Add comment in paginate_listings/paginate_dealers explaining why async generators use explicit item-yield instead of yield from. Co-Authored-By: Claude Sonnet 4.6 --- src/visor/__init__.py | 9 ++- src/visor/_pagination.py | 2 + src/visor/models/facets.py | 14 +++-- tests/test_exports.py | 1 + tests/test_models.py | 126 ++++++++++++++++++++++++++++++++++++- 5 files changed, 144 insertions(+), 8 deletions(-) diff --git a/src/visor/__init__.py b/src/visor/__init__.py index 6b69f30..db70b41 100644 --- a/src/visor/__init__.py +++ b/src/visor/__init__.py @@ -31,7 +31,13 @@ DealersPage, DealerSummary, ) -from visor.models.facets import FacetBucket, FacetsData, FacetsFilter, FacetsResponse +from visor.models.facets import ( + FacetBucket, + FacetsData, + FacetsFilter, + FacetSort, + FacetsResponse, +) from visor.models.listings import ( ListingDetail, ListingsFilter, @@ -79,6 +85,7 @@ "FacetsData", "FacetsFilter", "FacetsResponse", + "FacetSort", # listings "ListingDetail", "ListingsFilter", diff --git a/src/visor/_pagination.py b/src/visor/_pagination.py index eb097d2..5bb93fc 100644 --- a/src/visor/_pagination.py +++ b/src/visor/_pagination.py @@ -29,6 +29,7 @@ async def paginate_listings( *, max_pages: int | None = None, ) -> AsyncGenerator[ListingSummary, None]: + # Async generators cannot use `yield from`, so items are yielded one by one. if max_pages is not None and max_pages <= 0: return current = filter if filter is not None else ListingsFilter() @@ -51,6 +52,7 @@ async def paginate_dealers( *, max_pages: int | None = None, ) -> AsyncGenerator[DealerSummary, None]: + # Async generators cannot use `yield from`, so items are yielded one by one. if max_pages is not None and max_pages <= 0: return current = filter if filter is not None else DealerFilter() diff --git a/src/visor/models/facets.py b/src/visor/models/facets.py index 487eb71..1642c6e 100644 --- a/src/visor/models/facets.py +++ b/src/visor/models/facets.py @@ -1,4 +1,4 @@ -from typing import Literal, TypeAlias +from enum import Enum from pydantic import Field, field_validator, model_validator @@ -37,7 +37,13 @@ "days_on_market", } -FacetSort: TypeAlias = Literal["count", "-count", "metric", "-metric"] + +class FacetSort(str, Enum): + COUNT = "count" + COUNT_DESC = "-count" + METRIC = "metric" + METRIC_DESC = "-metric" + # Facets that return range histograms; all others are categorical (bucket lists). RANGE_FACET_NAMES = {"price", "msrp", "miles", "days_on_market"} @@ -50,7 +56,7 @@ class FacetsFilter(ListingsFilterBase): facets: list[str] facet_value_limit: int | None = None metric: str | None = None - sort: FacetSort = "-count" + sort: FacetSort = FacetSort.COUNT_DESC @field_validator("facets") @classmethod @@ -85,7 +91,7 @@ def to_params(self) -> dict[str, str]: params["facet_value_limit"] = str(self.facet_value_limit) if self.metric is not None: params["metric"] = self.metric - params["sort"] = self.sort + params["sort"] = self.sort.value return params diff --git a/tests/test_exports.py b/tests/test_exports.py index ea302b8..3c42299 100644 --- a/tests/test_exports.py +++ b/tests/test_exports.py @@ -45,6 +45,7 @@ "FacetsResponse", "FacetsData", "FacetBucket", + "FacetSort", # shared models "Pagination", "DealerRef", diff --git a/tests/test_models.py b/tests/test_models.py index 8d72334..1f0aae2 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -20,6 +20,8 @@ ) from visor.models.facets import ( FacetsData, + FacetsFilter, + FacetSort, FacetsResponse, ) from visor.models.listings import ( @@ -162,6 +164,92 @@ def test_vehicle_build_options_null() -> None: assert build.options is None +# --------------------------------------------------------------------------- +# Include-field semantics: None / [] / [...] +# +# Fields backed by include= follow a three-way contract: +# None — section not requested; API omitted or sent null +# [] — section requested but empty +# [...] — section requested and populated +# --------------------------------------------------------------------------- + + +def test_listing_detail_price_history_none_when_not_requested() -> None: + data = { + "id": "a", + "vin": "VIN1", + "status": "active", + "inventory_type": "new", + "dealer": DEALER_REF_DATA, + "vehicle": VEHICLE_RECORD_DATA, + } + ld = ListingDetail.model_validate(data) + assert ld.price_history is None + + +def test_listing_detail_price_history_empty_list_when_requested_but_empty() -> None: + data = { + "id": "a", + "vin": "VIN1", + "status": "active", + "inventory_type": "new", + "dealer": DEALER_REF_DATA, + "vehicle": VEHICLE_RECORD_DATA, + "price_history": [], + } + ld = ListingDetail.model_validate(data) + assert ld.price_history == [] + + +def test_listing_detail_price_history_populated() -> None: + data = { + "id": "a", + "vin": "VIN1", + "status": "active", + "inventory_type": "new", + "dealer": DEALER_REF_DATA, + "vehicle": VEHICLE_RECORD_DATA, + "price_history": [{"date": "2026-01-01", "price": 35000}], + } + ld = ListingDetail.model_validate(data) + assert ld.price_history is not None + assert len(ld.price_history) == 1 + assert ld.price_history[0].price == 35000 + + +def test_listing_snapshot_price_history_empty_list_when_requested_but_empty() -> None: + snap = ListingSnapshot.model_validate( + { + "id": "s1", + "inventory_type": "new", + "dealer": DEALER_REF_DATA, + "price_history": [], + } + ) + assert snap.price_history == [] + + +def test_vehicle_build_options_empty_list_when_requested_but_empty() -> None: + build = VehicleBuild.model_validate( + {"year": 2026, "make": "Toyota", "model": "Camry", "options": []} + ) + assert build.options == [] + + +def test_vehicle_build_options_populated() -> None: + build = VehicleBuild.model_validate( + { + "year": 2026, + "make": "Toyota", + "model": "Camry", + "options": [{"code": "X1", "name": "Sunroof", "msrp": 1200.0}], + } + ) + assert build.options is not None + assert len(build.options) == 1 + assert build.options[0].code == "X1" + + # --------------------------------------------------------------------------- # ListingsPage # --------------------------------------------------------------------------- @@ -302,6 +390,41 @@ def test_facets_data_defaults() -> None: assert fd.stats == {} +# --------------------------------------------------------------------------- +# FacetSort enum +# --------------------------------------------------------------------------- + + +def test_facet_sort_default_is_count_desc() -> None: + f = FacetsFilter(facets=["make"]) + assert f.sort is FacetSort.COUNT_DESC + assert f.sort.value == "-count" + + +def test_facet_sort_explicit_enum_accepted() -> None: + f = FacetsFilter(facets=["make"], sort=FacetSort.COUNT) + assert f.sort is FacetSort.COUNT + assert f.to_params()["sort"] == "count" + + +def test_facet_sort_raw_string_coerces() -> None: + # Pydantic coerces raw strings matching enum values in lax mode. + f = FacetsFilter(facets=["make"], sort="-count") # type: ignore[arg-type] + assert f.sort is FacetSort.COUNT_DESC + + +def test_facet_sort_all_wire_values() -> None: + expected = { + FacetSort.COUNT: "count", + FacetSort.COUNT_DESC: "-count", + FacetSort.METRIC: "metric", + FacetSort.METRIC_DESC: "-metric", + } + for member, wire in expected.items(): + f = FacetsFilter(facets=["make"], sort=member) + assert f.to_params()["sort"] == wire + + # --------------------------------------------------------------------------- # DealerSummary / DealerDetail / DealersPage # --------------------------------------------------------------------------- @@ -429,9 +552,6 @@ def test_dealer_filter_dealer_id_100_ok() -> None: # --------------------------------------------------------------------------- -from visor.models.facets import FacetsFilter # noqa: E402 - - def test_facets_filter_value_limit_max_100() -> None: with pytest.raises(ValidationError, match="facet_value_limit maximum is 100"): FacetsFilter(facets=["make"], facet_value_limit=101) From ad8357aa0b948b98caa7497ac278e08cc1413798 Mon Sep 17 00:00:00 2001 From: Andrew Miller Date: Wed, 17 Jun 2026 12:17:49 -0500 Subject: [PATCH 2/4] Add release_gate guards for ListingSummary include-field raw shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live probe (2026-06-17) confirmed the API omits price_history and options entirely from ListingSummary when include= is not passed — not null, not []. Pydantic default_factory=list is correct and unambiguous. Two release_gate integration tests now guard this: if the API ever starts sending null or [] for these fields without include=, the tests fail and signal that the model type needs review. Backlog P2 updated with findings. Co-Authored-By: Claude Sonnet 4.6 --- tests/integration/test_int_listings.py | 62 ++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/integration/test_int_listings.py b/tests/integration/test_int_listings.py index cac25ea..ef8699d 100644 --- a/tests/integration/test_int_listings.py +++ b/tests/integration/test_int_listings.py @@ -95,3 +95,65 @@ def test_get_listing_with_price_history( def test_get_listing_with_options(client: VisorClient, sample_listing_id: str) -> None: listing = client.get_listing(sample_listing_id, include=["options"]) assert isinstance(listing.vehicle.build.options, list) + + +# --------------------------------------------------------------------------- +# ListingSummary include-field raw-shape guards +# +# Background: ListingSummary.price_history and ListingSummary.options use +# list[...] with default_factory=list. That type does NOT accept null — if +# the API ever sends null for these fields without include=, Pydantic will +# raise a ValidationError at parse time. +# +# We verified via live probe (2026-06-17) that the API omits both fields +# entirely from the raw JSON when include= is not passed. Pydantic's +# default_factory=list kicks in, so listing.price_history == [] means +# "field was absent from response", not "requested and empty". +# +# These tests re-verify that invariant on every release-gate run. If either +# assertion fails it means the API started sending null (model type is wrong) +# or [] (model type is technically fine but semantics become ambiguous — +# revisit whether to make the field Optional so callers can distinguish +# "not requested" from "requested but empty"). +# --------------------------------------------------------------------------- + +_SENTINEL = object() + + +def _check_include_field_raw(client: VisorClient, field: str) -> None: + """Assert field is absent (not null, not []) in raw listings without include=.""" + raw = client._transport.get("/listings", {"limit": "5"}) + items: list[dict] = raw.get("data", []) + assert items, "No listings in response" + for item in items: + val = item.get(field, _SENTINEL) + assert val is _SENTINEL, ( + f"ListingSummary.{field} unexpectedly present in raw response " + f"without include=: {val!r}. " + f"If null: change type to list[...] | None. " + f"If []: field is now always sent; consider Optional for caller clarity." + ) + + +@pytest.mark.integration +@pytest.mark.release_gate +def test_listing_summary_price_history_absent_without_include( + client: VisorClient, +) -> None: + """price_history must be absent from raw JSON when include= is not passed. + + Verified live 2026-06-17: API omits the key entirely; Pydantic default [] + applies. Failure here means the API changed shape — re-evaluate the model. + """ + _check_include_field_raw(client, "price_history") + + +@pytest.mark.integration +@pytest.mark.release_gate +def test_listing_summary_options_absent_without_include(client: VisorClient) -> None: + """options must be absent from raw JSON when include=options is not passed. + + Verified live 2026-06-17: API omits the key entirely; Pydantic default [] + applies. Failure here means the API changed shape — re-evaluate the model. + """ + _check_include_field_raw(client, "options") From a0d9a4df137343c3ce8542cfc580b8d68428f0da Mon Sep 17 00:00:00 2001 From: Andrew Miller Date: Wed, 17 Jun 2026 12:22:09 -0500 Subject: [PATCH 3/4] Document FacetSort change and include-field semantics CHANGELOG: add Unreleased entries for the FacetSort enum conversion and the FacetsFilter.sort default/serialization fix. README: add Optional include fields section explaining the None/[]/[...] semantics for price_history and options, with a usage example and a note that ListingSummary variants are not include-backed. Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 13 +++++++++++++ README.md | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd4d9fe..a0659a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `FacetSort` enum (`str, Enum` with `COUNT`, `COUNT_DESC`, `METRIC`, `METRIC_DESC`) replaces + the previous `Literal` type alias, matching the `SortOrder` pattern used by `ListingsFilter`. + Raw string values (e.g. `"-count"`) continue to validate through Pydantic's lax coercion. + `FacetSort` is now exported from the `visor` package root. +- `FacetsFilter.sort` default changed from the string literal `"-count"` to `FacetSort.COUNT_DESC`. + +### Fixed + +- `FacetsFilter.to_params()` now serializes `sort` via `.value` (was relying on implicit + `str` coercion; behaviour is identical but now explicit and consistent with `ListingsFilter`). + ## [0.1.1] - 2026-06-13 ### Fixed diff --git a/README.md b/README.md index bbac01b..63a11c4 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,38 @@ filter = ListingsFilter( All responses — `ListingsPage`, `ListingDetail`, `VinDetail`, etc. — are Pydantic v2 models. Access fields as attributes and use standard Pydantic methods (`.model_dump()`, `.model_json_schema()`, etc.) as needed. +### Optional include fields + +Some fields are only populated when you request them via `include=`. The API uses three distinct states that the SDK preserves: + +| Value | Meaning | +|---|---| +| `None` | Section not requested — `include=` was not passed, or the API omitted the field | +| `[]` | Section requested but no data exists | +| `[...]` | Section requested and populated | + +Fields that follow this pattern: + +- `ListingDetail.price_history` — pass `include=["price_history"]` to `get_listing()` +- `ListingSnapshot.price_history` — pass `include=["price_history"]` to `lookup_vin()` +- `VehicleBuild.options` — pass `include=["options"]` to `get_listing()` or `lookup_vin()` + +```python +listing = client.get_listing(listing_id, include=["price_history"]) + +if listing.price_history is None: + print("price history was not requested") +elif listing.price_history == []: + print("requested but no price changes recorded") +else: + for entry in listing.price_history: + print(entry.date, entry.price) +``` + +`ListingSummary.price_history` and `ListingSummary.options` (returned by `filter_listings()`) +default to `[]` and are not include-backed — the API omits them entirely when `include=` is not +passed, so `[]` reliably means "not present in response" for summary listings. + ## Error handling All methods raise typed exceptions from `visor.exceptions`. The SDK does not retry automatically — `RateLimitError.retry_after` gives you the hint to build your own retry logic. From ccccc7c147418101f8bacc005a3f5b83ecebf6e2 Mon Sep 17 00:00:00 2001 From: Andrew Miller Date: Wed, 17 Jun 2026 12:28:07 -0500 Subject: [PATCH 4/4] Fix CHANGELOG classification and README summary-field note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG: split FacetSort Unreleased entry into Added (root export) and Changed (enum conversion, sort default); Fixed entry is unchanged. README: correct the include-field note for ListingSummary — those fields ARE include-backed via ListingsFilter(include=...) but default to [] when the key is absent, so callers cannot distinguish not-requested from requested-but-empty on summary listings. Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 12 ++++++++---- README.md | 8 +++++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0659a9..41a2ae9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- `FacetSort` enum (`str, Enum` with `COUNT`, `COUNT_DESC`, `METRIC`, `METRIC_DESC`) replaces - the previous `Literal` type alias, matching the `SortOrder` pattern used by `ListingsFilter`. - Raw string values (e.g. `"-count"`) continue to validate through Pydantic's lax coercion. - `FacetSort` is now exported from the `visor` package root. +- `FacetSort` is now exported from the `visor` package root. + +### Changed + +- `FacetSort` converted from a `Literal` type alias to a `str, Enum` + (`COUNT`, `COUNT_DESC`, `METRIC`, `METRIC_DESC`), matching the `SortOrder` pattern used by + `ListingsFilter`. Raw string values (e.g. `"-count"`) continue to validate through Pydantic's + lax coercion. - `FacetsFilter.sort` default changed from the string literal `"-count"` to `FacetSort.COUNT_DESC`. ### Fixed diff --git a/README.md b/README.md index 63a11c4..12af53b 100644 --- a/README.md +++ b/README.md @@ -187,9 +187,11 @@ else: print(entry.date, entry.price) ``` -`ListingSummary.price_history` and `ListingSummary.options` (returned by `filter_listings()`) -default to `[]` and are not include-backed — the API omits them entirely when `include=` is not -passed, so `[]` reliably means "not present in response" for summary listings. +`ListingSummary.price_history` and `ListingSummary.options` are also populated via +`ListingsFilter(include=...)`, but they default to `[]` when the API omits the key. +For summary listings, `[]` can mean either "not present in the response" or +"requested and empty"; use detail/VIN lookups if you need the three-state +`None` / `[]` / populated distinction. ## Error handling