Skip to content

feat(api)!: fluent request builders, opaque transport error, drop Client::new - #8

Merged
OpenSauce merged 3 commits into
mainfrom
api-shape-2026-08
Aug 11, 2026
Merged

feat(api)!: fluent request builders, opaque transport error, drop Client::new#8
OpenSauce merged 3 commits into
mainfrom
api-shape-2026-08

Conversation

@OpenSauce

@OpenSauce OpenSauce commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Second of four PRs preparing the 0.1.0 release. This is the one that freezes the public API, so everything breaking lands here.

Fluent request builders replace the param structs

SearchParams, ListParams, ModelListParams and UserListParams are gone. client.tones(), .created(), .favorited(), .models(tone_id) and .users() return builders that implement IntoFuture, so they're awaited directly — no .send().

// before
client.search(SearchParams { query: Some("plexi".into()), ..Default::default() }).await?;
// after
client.tones().query("plexi").await?;

// browse, which has no endpoint of its own — it's search with no query
client.tones().sort(ToneSort::DownloadsAllTime).page_size(24).await?;

Beyond ergonomics, this makes adding a filter a non-breaking change (a new builder method), which matters for an API that keeps adding them. #[non_exhaustive] on the old structs couldn't do that — it forbids ..Default::default() from other crates entirely.

reqwest leaves the public API

Error::Http now wraps an opaque HttpError exposing is_timeout(), is_connect() and status(), mirrored by Error::is_timeout()/is_connect(). reqwest has shipped four breaking majors in seven years — the most recent in December 2025 — and each would otherwise force a breaking release here plus lockstep upgrades for consumers.

Client::new() removed

It built a client on which every call failed with Unauthenticated (every endpoint needs a user access token), while being the most discoverable constructor in the crate. Client::builder(key) is the only path that works.

architecture is ArchitectureVersion, not u32

Upstream types.ts declares architecture?: number. It's wrong. Probed live on 2026-08-11:

architecture= Result
1 200, total 7,914
2 200, total 9,590
custom 200, total 1,233
banana 400 — "architecture must be '1', '2', or 'custom' when provided"

Three distinct result sets, and the API's own error message names the exact vocabulary ArchitectureVersion already models. As u32 the parameter both made custom unfilterable and let callers build requests the server rejects (.architecture(3) compiled, then 400'd). Input and output now share one type.

Second time types.ts has been wrong, after the platformformat rename.

Also

  • Page::has_next() / has_prev()
  • CLAUDE.md updated to describe the new surface

Verification

cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test — 57 passing, live tests ignored as designed.

🤖 Generated with Claude Code

…ent::new

Freezes the public surface ahead of the 0.1.0 publish. Three changes, all of
which are cheap now and breaking later.

Fluent builders replace the four param structs. `SearchParams`, `ListParams`,
`ModelListParams` and `UserListParams` are gone; `client.tones()`, `.created()`,
`.favorited()`, `.models(tone_id)` and `.users()` return builders implementing
`IntoFuture`, so they are awaited directly with no `.send()`. This removes the
`Some(..)` / `..Default::default()` noise from every call site and makes adding
a filter a non-breaking change, which matters for an API that adds them.

`tones()` covers browse and search alike. The API has no separate browse
endpoint — a "top tones" listing is the search endpoint with no query and a
sort — so one entry point matches what the service actually offers.

`reqwest::Error` leaves the public API behind an opaque `HttpError` newtype
exposing `is_timeout()`, `is_connect()` and `status()`, mirrored by
`Error::is_timeout()`/`is_connect()`. reqwest has shipped four breaking majors
in seven years, most recently in December 2025; each one would otherwise force
a breaking release of this crate and lockstep upgrades on consumers.

`Client::new()` is removed. Every endpoint requires a user access token, so it
built a client on which every call failed with `Unauthenticated` while being
the most discoverable constructor in the crate. `Client::builder(key)` is the
one path that works.

Also adds `Page::has_next()`/`has_prev()`, so paging a browse UI is not
arithmetic every consumer rewrites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 11, 2026 22:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prepares the crate for a 0.1.0 “public API freeze” by switching list/search endpoints to fluent request builders (awaitable via IntoFuture), removing Client::new(), and introducing an HttpError wrapper intended to keep reqwest out of the public surface while preserving useful transport predicates.

Changes:

  • Replaced *Params request structs with fluent request builders: tones(), created(), favorited(), models(tone_id), users() (await directly; no .send()).
  • Introduced HttpError + Error::{is_timeout,is_connect} helpers and re-exported the new public endpoint builder types.
  • Updated tests/examples/docs to the new API, and added Page::has_next() / has_prev() helpers.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/read_path.rs Updates wiremock tests to new fluent builders; adds pagination serialization coverage.
tests/oauth.rs Migrates OAuth refresh tests to client.tones().await style and builder construction.
tests/live_user.rs Updates live contract test to new created()/favorited() builders.
tests/live_public.rs Updates live contract tests to new tones()/models()/users() builders.
src/models/user.rs Removes UserListParams model param struct.
src/models/tone.rs Removes SearchParams/ListParams model param structs.
src/models/page.rs Adds Page::has_next() / has_prev() plus tests.
src/models/model.rs Removes ModelListParams model param struct.
src/models/mod.rs Stops re-exporting removed params structs.
src/lib.rs Updates crate docs; re-exports new endpoint builder types and HttpError.
src/error.rs Replaces Error::Http(reqwest::Error) with Error::Http(HttpError) and adds transport predicates.
src/endpoints/users.rs Converts Client::users(params) to Client::users() -> UserList fluent builder.
src/endpoints/tones.rs Converts search/list endpoints to ToneSearch / ToneList builders implementing IntoFuture.
src/endpoints/models.rs Converts Client::models(tone_id, params) to Client::models(tone_id) -> ModelList builder.
src/endpoints/mod.rs Makes endpoint submodules public for re-exporting builder types from crate root.
src/client.rs Removes Client::new() and updates tests accordingly.
examples/search_and_download.rs Updates example to tones().query(...).await and models(tone_id).await.
CLAUDE.md Updates repository guidance to reflect the new fluent surface and error approach.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/error.rs
Comment on lines +66 to +70
/// The HTTP client behind this is an implementation detail and may be swapped or
/// version-bumped without a breaking release, so the underlying error type is not part of
/// this crate's public API. The predicates below cover what callers actually branch on.
#[derive(Debug)]
pub struct HttpError(reqwest::Error);
OpenSauce and others added 2 commits August 11, 2026 23:51
Upstream types.ts declares `architecture?: number` on both SearchTonesParams
and ListModelsParams. That is wrong. Probing the live API on 2026-08-11:

  architecture=1       200, total 7914
  architecture=2       200, total 9590
  architecture=custom  200, total 1233
  architecture=banana  400, "architecture must be '1', '2', or 'custom'
                            when provided"

Three distinct result sets, and the API's own error message states the exact
vocabulary already modelled by ArchitectureVersion. Typing the parameter as
u32 both made `custom` unfilterable and let callers build requests the server
rejects — `.architecture(3)` compiled and 400'd.

Input and output now share one type: `Model::architecture_version` reads an
ArchitectureVersion, and searching for more like it takes the same value.

This is the second time types.ts has been wrong (see the platform -> format
rename), which is why CLAUDE.md treats it as a trigger to investigate rather
than a source to sync from.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot and a review pass over #8 found several things that would have been
frozen at publish. All of them are cheap now and breaking later.

reqwest is genuinely out of the public API. The `From<reqwest::Error>` impls
for `HttpError` and `Error` were public — trait impls always are — so a
downstream crate could convert a reqwest error into ours, putting reqwest back
in the semver surface the newtype exists to remove. Replaced by a `pub(crate)`
constructor and a `pub(crate) TransportResultExt` used at the `?` sites.
(Copilot also flagged the tuple field type itself; that one is a non-issue,
private field types are not public API.)

Response structs are `#[non_exhaustive]`: Tone, Model, Page, User, PublicUser,
EmbeddedUser, Make, Tag, Tokens, plus the struct-like Error variants. The whole
model layer is built on the premise that this API adds fields without notice;
without this, one added field forces a breaking release. Input types
(AuthorizeOptions) stay exhaustive so `..Default::default()` keeps working.
`download_url`/`download_url_to` are added so a caller who persisted a
`model_url` rather than a whole Model can still fetch it.

Builders own a Client clone instead of borrowing it. Client is Arc-backed and
already Clone, so the cost is a refcount bump and the futures become 'static —
spawnable for background prefetch, storable in a struct. Going from
`ToneSearch<'a>` to `ToneSearch` after publish would have been breaking.

Builders are `#[must_use]`, Clone and Debug. Dropping an un-awaited builder was
silent, a regression against the old `async fn` which warned via the future's
own must_use; verified the warning fires again. Clone lets a configured search
walk its pages, which pairs with the `has_next()` added in this PR.

`AuthorizeOptions::architecture` becomes ArchitectureVersion, missed when every
other `architecture` was retyped. It is the same parameter with the same
'1' | '2' | 'custom' vocabulary, so `Some(3)` compiled and 400'd at runtime.

`HttpError::status()` is removed: reqwest only populates it via
`error_for_status`, which this crate never calls, so it could only ever return
None. Statuses arrive as `Error::Status { code, .. }` from http.rs instead.

`Error::Http` is `#[error(transparent)]`. It previously interpolated the source
into its own Display while also exposing it via source(), so chain printers
showed the same message twice.

Tests cover what changed: sort serialization (uncovered since SearchParams was
removed), append-vs-replace semantics, a bare request sending no query at all,
ArchitectureVersion::Other on the wire, a real connect failure through
is_connect(), Clone-based paging, and spawnability. 64 passing.

CONTEXT.md is committed so CLAUDE.md's reference to it resolves.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@OpenSauce
OpenSauce merged commit 540b6cd into main Aug 11, 2026
1 check passed
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.

2 participants