Repository navigation
Add mapbox places, full detail for one or more Search Box result ids - #81
mattpodwysocki wants to merge 3 commits into
Conversation
zmofei
left a comment
There was a problem hiding this comment.
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}: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| #### Outputs | ||
|
|
||
| Captured live, the same Ferry Building above plus a second real place: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| - `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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
0694013 to
9bf5628
Compare
zmofei
left a comment
There was a problem hiding this comment.
Thanks, the one-command shape looks good. I tested 9bf5628 against the real API.
Before merge:
- Please rebase on main. #43, #44 and #45 are merged, and 8 files now conflict. main already has
FLATTENED_SERVICESfrom #43, so please keep that one and addplacesto it. - 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": |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| ### `mapbox places` | ||
|
|
||
| One or more full place records. No subcommand: earlier versions of this |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Dropped both, this section is now just the product facts.
| field: field.to_string(), | ||
| arg_name: arg_name.to_string(), | ||
| max, | ||
| description: description.map(|text| format!("{text} Up to {max}.")), |
There was a problem hiding this comment.
Optional: mapbox places --schema says "up to 100. Up to 100." The yaml description and this line both add it. Please keep only one.
There was a problem hiding this comment.
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.
9bf5628 to
2df146a
Compare
|
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. |
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>
…re, drop duplicate schema text, help groups
2df146a to
b988f2b
Compare
What
mapbox places <mapbox-id>..., full place detail — hours, phone, website, photos, address, coordinates, activity data — for one or moremapbox_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) andplaces 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;batchis the one that survives, flattened to the bareplacescommand, 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_ARRAYinspec.rs: a JSON body's array field exposed as one or more positional arguments collected into it, in place of--data. clap'snum_args(1..=100)enforces the id limit client-side, before the request goes out. A206partial result (some ids unresolved) needed no new handling:status.is_success()already treats any 2xx as success.Since the
FLATTENED_SERVICES/declared_commandmechanism 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 inspec.rsthat 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 forwardto find two real places (Ferry Building, Golden Gate Bridge), fetched one by id, then both at once.The
-o textoutput shown indocs/commands.mdis 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 textwas caught as wrong in review:{"results": [...]}is a one-key object wrapping an array, whichrender_human'swrapped_listturns 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
placesis flattened and variadic, and 3 dry-run integration tests for one id / many ids / the 100-id limit),cargo fmt --checkandcargo clippy --all-targets -- -D warningsall clean.🤖 Generated with Claude Code