Skip to content

Add mapbox places, full detail for one or more Search Box result ids - #81

Open
mattpodwysocki wants to merge 3 commits into
mainfrom
feat/places-api
Open

mattpodwysocki wants to merge 3 commits into
mainfrom
feat/places-api

Conversation

@mattpodwysocki

@mattpodwysocki mattpodwysocki commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

What

mapbox places <mapbox-id>..., full place detail — hours, phone, website, photos, address, coordinates, activity data — for one or more mapbox_ids a Search Box API result already returned, up to 100 in one call.

Reworked in review: this used to be split into places get (one id) and places batch (a JSON array of ids), reviewed as the wrong shape for any API with a single-item GET and a batch POST of the same thing. A caller should not have to choose between one id and a hand-typed JSON array. The GET operation is gone from the spec entirely, not hidden; batch is the one that survives, flattened to the bare places command, and always sends the batch request, one id or many — so a single id reads the same way as a hundred.

New general mechanism, VARIADIC_BODY_ARRAY in spec.rs: a JSON body's array field exposed as one or more positional arguments collected into it, in place of --data. clap's num_args(1..=100) enforces the id limit client-side, before the request goes out. A 206 partial result (some ids unresolved) needed no new handling: status.is_success() already treats any 2xx as success.

Since the FLATTENED_SERVICES/declared_command mechanism this needs comes from the directions/isochrone/map-match/matrix stack, which hasn't merged yet, it's ported into this branch directly rather than waiting on that stack — small and self-contained, and will need a routine merge-conflict resolution (additive) once that stack lands, same as every other table in spec.rs that stack also touches.

Hand-authored into custom-openapi/ since no upstream spec exists yet. Places is Public Preview, with a 1000-records-per-account monthly quota.

Verification

Smoke-tested against production in the original pass: used search forward to find two real places (Ferry Building, Golden Gate Bridge), fetched one by id, then both at once.

The -o text output shown in docs/commands.md is the one thing not re-verified against a live call this pass — this environment has no network access to the real API. Instead, it's the page's own previously-captured real Ferry Building record run back through this CLI's actual output-rendering code (not guessed), which is how the old page's claim of raw JSON for -o text was caught as wrong in review: {"results": [...]} is a one-key object wrapping an array, which render_human's wrapped_list turns into a table. Said so explicitly on the page rather than presenting it as a fresh capture.

Full suite (535 tests, 3 new: a spec.rs unit test for the schema validation, one confirming places is flattened and variadic, and 3 dry-run integration tests for one id / many ids / the 100-id limit), cargo fmt --check and cargo clippy --all-targets -- -D warnings all clean.

🤖 Generated with Claude Code

@mattpodwysocki
mattpodwysocki requested a review from a team as a code owner October 6, 2026 14:14

@zmofei zmofei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. We want a different command shape for Places, so this needs changes before merge. Details inline.

- url: https://api.mapbox.com
description: Places API
paths:
/places/v1/details/retrieve/{mapbox_id}:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We want one command, mapbox places <MAPBOX_ID>..., not get and batch. It should take one or more ids and call the right endpoint. This is the rule we want for any API with a single and a batch endpoint: one command, and the CLI picks the request inside. Users and agents should not have to choose between two commands or write the batch JSON by hand. Existing pairs like geocoder forward/batch came before this rule. For Places, I suggest always using the POST endpoint so the output has one shape, and deciding what happens with more than 100 ids and with a 206 partial result.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done: one command, mapbox places <mapbox-id>..., always uses the batch endpoint even for a single id. Went with always-POST, as you suggested. The 100-id cap is enforced client-side via clap's num_args before the request goes out, and 206 partial results needed no new handling since the existing status.is_success() check already treats any 2xx as success.

Comment thread docs/commands.md Outdated

#### Outputs

Captured live, the same Ferry Building above plus a second real place:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The -o text column for places batch shows JSON, but {"results": [...]} has one key, so the CLI renders it as a table. Please re-capture the text output from a real run, or mark which parts were edited.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, confirmed by actually running the real rendering code against this page's own captured data: {"results": [...]} is a one-key object wrapping an array, which render_human unwraps into a table. Re-captured the -o text column that way (disclosed as not a fresh live call, since this environment has no network access right now, but run through the real renderer, not guessed).

Comment thread CHANGELOG.md Outdated
- `mapbox places get`/`batch`, full place detail — hours, phone, website,
photos, address, coordinates, activity data — by the `mapbox_id` a
Search Box API result already returned. Hand-authored into
`custom-openapi/` for the same reason this session's other additions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: "this session's other additions" and "discovered while writing ..." describe how the PR was made, not the product. Please keep only user-facing facts here, and mention that Places is Public Preview with a monthly quota.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reworded to drop the process language and describe the product instead, and added that Places is Public Preview with a 1000-records-per-account monthly quota.

@zmofei zmofei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, the one-command shape looks good. I tested 9bf5628 against the real API.

Before merge:

  1. Please rebase on main. #43, #44 and #45 are merged, and 8 files now conflict. main already has FLATTENED_SERVICES from #43, so please keep that one and add places to it.
  2. When no id resolves, please exit non-zero. See the inline comment.

The other comments are optional. Optional: the title still says places get/batch.

I'll review again after the rebase.

`popularity`, each 0-1), and where available `brand`,
`opening_hours`, `phone`, `photos`, `website`, `building`, and
`telemetry` (hourly activity by day of week).
"206":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Exit 0 when some ids are missing makes sense: the caller still gets the ones that resolved. But when no id resolves, mapbox places <id> exits 0 with "results": [] and prints nothing to stderr. Before, places get failed with 404. Please exit non-zero in this case. Exit codes are part of the contract, so this is easier to decide before release.

Optional: also print the missing/unprocessed ids to stderr.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: when no id resolves at all, the CLI now exits non-zero instead of reporting success with an empty results. The ids that didn't resolve print to stderr too (took your optional suggestion). Tested against production with a well-formed but nonexistent id:

$ mapbox places <made-up-id>
No id resolved: <made-up-id>
Error: No id resolved. See the ids above, or re-check them against a Search Box result.
$ echo $?
1

Partial success (some ids resolve, some don't) still exits 0, unchanged.

Comment thread docs/commands.md Outdated
Building San Francisco"` — run through this CLI's own output rendering
again to show both modes honestly for the one-command shape, rather than
reused verbatim from the old two-command page. Not re-captured from a
live call: this environment has no network access to the real API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Optional: the API is reachable now, so please capture this from a real run. The real -o text table also has BRAND, CREATED_AT and UPDATED_AT columns.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-captured for real, both the single-id and two-id cases, including BRAND/CREATED_AT/UPDATED_AT. Also captured the 206/missing case live this time (paired a real id with a well-formed one that doesn't exist) and noticed something worth documenting: since that response has two top-level keys (missing + results) rather than one, -o text falls back to pretty JSON instead of the usual one-key-object-to-table unwrap. Wrote that up in the Outputs section too.

Comment thread docs/commands.md Outdated

### `mapbox places`

One or more full place records. No subcommand: earlier versions of this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Optional: "earlier versions of this page", "discovered while writing" (line 36) and "this environment has no network access" describe how the PR was made. Please keep only what a user needs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Dropped both, this section is now just the product facts.

Comment thread src/spec.rs Outdated
field: field.to_string(),
arg_name: arg_name.to_string(),
max,
description: description.map(|text| format!("{text} Up to {max}.")),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Optional: mapbox places --schema says "up to 100. Up to 100." The yaml description and this line both add it. Please keep only one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, it was the VariadicBodyArray code appending its own "Up to {max}." on top of whatever the yaml's description already said. Removed the code-side append since the yaml already states the max; --schema and --help both show it once now.

@mattpodwysocki mattpodwysocki changed the title Add mapbox places get/batch Add mapbox places, full detail for one or more Search Box result ids Oct 9, 2026
@mattpodwysocki

Copy link
Copy Markdown
Contributor Author

Rebased on main (directions/isochrone/map-match/matrix are all in now, plus #91's help-groups fix) and added places to the existing FLATTENED_SERVICES/Search help group rather than keeping a separate copy.

Also fixed: no-id-resolved now exits non-zero, -o text/json re-captured from real API calls (BRAND/CREATED_AT/UPDATED_AT included, plus a live 206 capture), the duplicate "Up to 100" in --schema, and the process-language notes in docs/commands.md and CHANGELOG.md. Title updated to drop the old get/batch naming.

Ready for another look.

@mattpodwysocki
mattpodwysocki requested a review from zmofei October 9, 2026 16:10
zmofei
zmofei previously approved these changes Oct 9, 2026
mattpodwysocki and others added 3 commits October 9, 2026 15:04
Eighth and final API from the original candidate list — completes it.
Hand-authored into custom-openapi/ since openapi-specs has no spec for
this API either.

Full detail for a place — hours, phone, website, photos, address,
coordinates, activity data — by the mapbox_id a Search Box API result
already returned. This API has no search/suggest of its own: get resolves
one id, batch resolves up to 100 in one call via --data '{"ids": [...]}',
the same shape styles create already uses for a body with no sensible
per-field flag.

No profile path parameter, and no listing/detail auto-link risk either:
batch is POST so it's never a candidate for the GET-only linker this
session's ev-charge-finder work just fixed a bug in.

Smoke-tested against production end to end: used search forward to find
two real places (Ferry Building, Golden Gate Bridge), fetched one by id
with get, then fetched both at once with batch.

Also discovered while testing: this environment's token now has real
Search Box API access, which docs/commands.md's search section previously
said it lacked (true when that was written, not true now). Updated the
page's own accounting of what's live-verified vs. not to say so, without
re-capturing search's four Outputs sections in this same change — flagged
as a worthwhile follow-up, not done here.

489 tests, fmt and clippy clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mapbox places <mapbox-id>... replaces places get/places batch: reviewed
as the wrong shape for any API with a single-item GET and a batch POST of
the same thing. A caller should not have to choose between one id and a
hand-typed JSON array. The GET operation is gone from the spec entirely,
not hidden; batch is the one that survives, flattened to the bare
`places` command (new `FLATTENED_SERVICES`/`declared_command` mechanism,
ported into this branch since the directions/isochrone/map-match/matrix
stack that introduces it hasn't merged yet), and always sends the batch
request, one id or many.

New general mechanism, VARIADIC_BODY_ARRAY in spec.rs: a JSON body's
array field exposed as one or more positional arguments collected into
it, in place of --data, with its description read off the same schema
`properties.<field>.description` body_field_parameters already reads for
BODY_FIELD_FLAGS. clap's num_args(1..=max) enforces the 100-id limit
client-side, before the request goes out. A 206 partial result (some ids
unresolved) needed no new handling: status.is_success() already treats
any 2xx as success.

Fixed the -o text documentation while rewriting this page: the old
places batch example showed raw JSON for -o text, but {"results": [...]}
is a one-key object wrapping an array, which render_human's wrapped_list
already turns into a table — verified by actually running the real
rendering code against the page's own previously-captured production
data, not guessed. Nested fields (coordinates, address, score) and arrays
(categories, photos) are dropped from table columns, same rule every
table on this page follows; documented that explicitly since the old
page didn't show a wide enough record to make it obvious.

Also reworded the CHANGELOG entry to drop process language ("this
session's other additions", "discovered while writing") in favor of
product facts, and added that Places is Public Preview with a
1000-records-per-account monthly quota.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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