From 384aa0552d47d9baf2af59af5a5ece3d7acc7804 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 23 Oct 2026 11:04:48 +0300 Subject: [PATCH 1/6] Let trailing optional path positionals be left off, and regroup feedback static get-image/get-tile and tilesets get-tile required every path positional, so highRes, format and quality had to be passed as "". The run of omittable positionals at the end is now optional, and the executor sends an absent one as "". Earlier optional positionals say in their help how to skip them. feedback moves from the Search help group to Maps and data, with a description that says it reads as well as submits. --- CHANGELOG.md | 6 +++++ docs/commands.md | 31 +++++++++++---------- src/executor.rs | 11 ++++++-- src/help_layout.rs | 8 +++--- src/main.rs | 58 +++++++++++++++++++++++++++++++++++++--- src/schema.rs | 56 +++++++++++++++++++++++++++++++------- tests/output_contract.rs | 40 ++++++++++++++++++++++++++- 7 files changed, 178 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1063b6..2ec87c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,12 @@ that may never merge. They are not releases and are not listed here. API for the per-browser, per-country and per-host breakdown, which it now sends only on request. Without the flag the request is unchanged, and each product's `dimensions` in `-o json` is `{}`. +- `static get-image`, `static get-tile` and `tilesets get-tile` no longer + need their trailing optional positionals: ``, `` and + `` can be left off instead of passed as `""`. Writing `""` still + works, so existing scripts are unaffected. `--schema` now reports these + arguments as not required. +- `feedback` moved from the Search help group to Maps and data. ## 1.1.1 - 2026-10-22 diff --git a/docs/commands.md b/docs/commands.md index f15ab97..dbfaeee 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2697,9 +2697,11 @@ Tiles merged into this one command group (#116); `get-image` and `get-tile` are what were `static-images get-static-image` and `static-tiles get-static-tile`. -`` takes a **leading dot** — `.png`, `.jpeg`, `.webp`, or `""` for -the style default. `` is `@2x` or `""`. `` (`get-image` -only) is a marker/path/GeoJSON expression, or `""` for none. +`` takes a **leading dot** — `.png`, `.jpeg`, `.webp`, or left off +for the style default. `` is `@2x`, or left off. Both come last, so +either can be left off the end of the line; `""` still works in their place. +`` (`get-image` only) is a marker/path/GeoJSON expression, or `""` +for none. `""` for the overlay drops the segment entirely rather than sending an empty one, so a plain map image needs no overlay expression. @@ -2710,11 +2712,11 @@ A map image centered on a point. #### Parameters -Eleven positionals, in order: ` - `. +Nine positionals, then two optional ones, in order: ` + [highRes] [format]`. `` and `` default to `0` at the API, but the CLI requires -both as integers — pass `0 0` rather than `"" ""`. +both as numbers — pass `0 0` rather than `"" ""`. | Parameter | Effect | | --- | --- | @@ -2726,7 +2728,7 @@ both as integers — pass `0 0` rather than `"" ""`. #### Examples ```sh -mapbox static get-image streets-v12 "" 24.94 60.16 12 0 0 600 400 "" .png \ +mapbox static get-image streets-v12 "" 24.94 60.16 12 0 0 600 400 \ --username mapbox > map.png mapbox static get-image streets-v12 "pin-s+555555(24.94,60.16)" \ @@ -2762,9 +2764,10 @@ One raster tile rendered from a style, rather than from a tileset. #### Parameters -Seven positionals: ` `. +Five positionals, then two optional ones: ` +[highRes] [format]`. -`` is `512` (the default) or `256`. 512 px tiles at zoom *z* cover +`` is `512` (the API's default) or `256`, and has to be written. 512 px tiles at zoom *z* cover the same ground as 256 px tiles at *z+1*, so 256 needs four times as many requests for the same area. @@ -2774,7 +2777,7 @@ requests for the same area. #### Examples ```sh -mapbox static get-tile streets-v12 512 2 1 1 "" .png \ +mapbox static get-tile streets-v12 512 2 1 1 \ --username mapbox > tile.png mapbox static get-tile streets-v12 256 12 2048 1361 "@2x" .png \ --username mapbox > tile@2x.png @@ -3301,13 +3304,13 @@ tile. ` ` are the tile coordinates. `` is `png`, `pngraw`, `jpg`, `jpeg` or `webp`. `` is appended to it rather than being a separate URL segment, so `jpg` with `70` -fetches `.jpg70`; pass `""` for the format's default. Both are positional, -in that order. +fetches `.jpg70`; leave it off for the format's default. Both are +positional, in that order. #### Examples ```sh -mapbox tilesets get-tile mapbox.satellite 2 1 1 png "" > tile.png +mapbox tilesets get-tile mapbox.satellite 2 1 1 png > tile.png mapbox tilesets get-tile mapbox.satellite 12 2048 1361 jpg 70 > tile.jpg70 ``` @@ -3331,7 +3334,7 @@ it to a file, e.g. `... > out.jpg`. ``` -$ mapbox tilesets get-tile mapbox.satellite 2 1 1 png "" > tile.png +$ mapbox tilesets get-tile mapbox.satellite 2 1 1 png > tile.png $ file tile.png tile.png: JPEG image data ``` diff --git a/src/executor.rs b/src/executor.rs index 38687d0..edc60ab 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -158,8 +158,15 @@ fn dispatch( } } - for param in &op.path_params { - if let Some(val) = matches.get_one::(¶m.arg_name) { + let omittable = crate::schema::omittable_tail(&op.path_params, &op.path_template); + for (param, omittable) in op.path_params.iter().zip(omittable) { + // An absent one is sent as `""`, which `omittable_tail` vouches for; + // anything else absent falls through to the missing-parameter error. + let val = matches + .get_one::(¶m.arg_name) + .map(String::as_str) + .or(omittable.then_some("")); + if let Some(val) = val { let safe = path_segment_for(&op.service, param, val)?; path = substitute_path_param(&path, ¶m.name, &safe, param.required); } diff --git a/src/help_layout.rs b/src/help_layout.rs index f1c7757..dc035c2 100644 --- a/src/help_layout.rs +++ b/src/help_layout.rs @@ -33,9 +33,11 @@ use clap::{Arg, Command}; const GROUPS: &[(&str, &[&str])] = &[ ( "Maps and data", - &["styles", "sprites", "fonts", "tilesets", "static"], + &[ + "styles", "sprites", "fonts", "tilesets", "static", "feedback", + ], ), - ("Search", &["search", "geocoder", "places", "feedback"]), + ("Search", &["search", "geocoder", "places"]), ( "Navigation", &["directions", "matrix", "isochrone", "map-match"], @@ -84,7 +86,7 @@ const DESCRIPTIONS: &[(&str, &str)] = &[ ), ( "feedback", - "Report wrong or missing map data, and list what was reported", + "Read and submit user reports of wrong or missing map data", ), ("directions", "Route between waypoints"), ( diff --git a/src/main.rs b/src/main.rs index 36c147f..5e085fc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -165,6 +165,34 @@ fn help_text(param: &spec::Parameter) -> String { .to_string() } +/// The API's value for a spec-optional path parameter that clap cannot take +/// `""` for, so a caller who wants the default knows what to write. The +/// specs say these in prose (`static get-image`'s description, `tilesize`'s +/// `default: 512`), which the parser does not read for a path parameter. +const PATH_PARAM_DEFAULTS: &[(&str, &str)] = + &[("bearing", "0"), ("pitch", "0"), ("tilesize", "512")]; + +/// A positional's help, plus how to skip it when the spec calls it optional +/// but clap still needs something written in its place. +fn path_help_text(param: &spec::Parameter, path_template: &str, omittable: bool) -> String { + let help = help_text(param); + let how_to_skip = if param.required || omittable { + None + } else if schema::omits_with_empty(param, path_template) { + Some("Pass \"\" to leave it out".to_string()) + } else { + PATH_PARAM_DEFAULTS + .iter() + .find(|(name, _)| *name == param.name) + .map(|(_, default)| format!("Pass {default} for the API's default")) + }; + match how_to_skip { + Some(note) if help.is_empty() => note, + Some(note) => format!("{help}. {note}"), + None => help, + } +} + /// The coordinate-shaped parameters whose value legitimately starts with `-` /// without being a bare number. /// @@ -235,11 +263,12 @@ fn build_operation_command(op: &spec::Operation) -> Command { }); } - for param in &op.path_params { + let omittable = schema::omittable_tail(&op.path_params, &op.path_template); + for (param, omittable) in op.path_params.iter().zip(omittable) { let mut arg = Arg::new(param.arg_name.clone()) - .required(true) + .required(!omittable) .allow_hyphen_values(takes_hyphen_leading_value(param)) - .help(help_text(param)); + .help(path_help_text(param, &op.path_template, omittable)); // Positionals get the same treatment as flags: clap renders the // possible values into the help itself, so the hand-rolled suffix @@ -2522,6 +2551,29 @@ mod tests { assert_eq!(seen, expected, "auth lost a subcommand"); } + /// Each default names a parameter that still needs one: spec-optional, + /// and something clap will not let a caller leave blank. One that the + /// specs dropped, or that became omittable, would print advice nobody + /// needs. + #[test] + fn every_path_param_default_is_still_needed() { + let specs = bundled_specs(); + for (name, _) in PATH_PARAM_DEFAULTS { + let needed = specs.iter().flat_map(|svc| &svc.operations).any(|op| { + op.path_params.iter().any(|param| { + param.name == *name + && !param.required + && !schema::omits_with_empty(param, &op.path_template) + }) + }); + assert!( + needed, + "PATH_PARAM_DEFAULTS lists `{name}`, which no spec-optional path \ + parameter needs any more — remove it" + ); + } + } + /// The same trap `no_generated_flag_shadows_a_global` catches, one level /// down. `--dry-run` is not global — it is added to the mutating /// operations individually — so that test cannot see it, and a spec that diff --git a/src/schema.rs b/src/schema.rs index b46f2ad..5a4163a 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -448,17 +448,17 @@ fn declared_command<'a>(app: &'a Command, service: &str, path: &[String]) -> Opt fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry { let declared = declared_command(app, &svc.name, &op.command_path); + let omittable = omittable_tail(&op.path_params, &op.path_template); let mut arguments: Vec = op .path_params .iter() - .map(|param| Argument { - // Every path positional is required, whatever the spec says, - // because `build_operation_command` makes it so: the value has to - // be written even where the spec calls the segment optional. What - // the spec's `required: false` buys is the right to write nothing - // *as* that value, which `omit_with_empty` reports rather than - // contradicting clap here. - required: true, + .enumerate() + .map(|(index, param)| Argument { + // Clap's answer, not the spec's: a spec-optional positional can + // only be left off at the end of the line, so one earlier on is + // still required here and `omit_with_empty` reports whether `""` + // stands in for it. + required: !omittable[index], location: Some("path"), omit_with_empty: omits_with_empty(param, &op.path_template), // Named as well as filled, the same reason `username_argument` @@ -582,7 +582,7 @@ fn parameter(param: &Parameter, kind: ArgKind) -> Argument { /// (`bearing`, `pitch`) or for an enum that does not list `""` among its /// values (`tilesize`) — those are spec-optional and, through this CLI, /// impossible to omit. And the URL that results has to be a URL. -fn omits_with_empty(param: &Parameter, path_template: &str) -> bool { +pub(crate) fn omits_with_empty(param: &Parameter, path_template: &str) -> bool { if param.required { return false; } @@ -596,6 +596,22 @@ fn omits_with_empty(param: &Parameter, path_template: &str) -> bool { clap_accepts_empty && leaves_a_usable_url(path_template, ¶m.name) } +/// Which path positionals may be left off the command line altogether. +/// +/// Clap can only drop a positional when every one after it can go too, so +/// this is the run at the end that [`omits_with_empty`] allows: the executor +/// sends an absent one exactly as it sends `""`. +pub(crate) fn omittable_tail(path_params: &[Parameter], path_template: &str) -> Vec { + let mut omittable = vec![false; path_params.len()]; + for (index, param) in path_params.iter().enumerate().rev() { + if !omits_with_empty(param, path_template) { + break; + } + omittable[index] = true; + } + omittable +} + /// Whether taking this placeholder out of the template leaves a path the API /// will read the way the caller meant. /// @@ -1348,6 +1364,28 @@ mod tests { )); } + /// Only the run at the end can be left off: clap cannot skip a + /// positional with a required one after it. + #[test] + fn only_the_trailing_run_of_omittable_positionals_is_optional() { + let required = |name| Parameter { + required: true, + ..optional(name) + }; + let params = [ + optional("overlay"), + required("zoom"), + required("w"), + required("h"), + optional("highRes"), + optional("format"), + ]; + assert_eq!( + omittable_tail(¶ms, "/static/{overlay}/{zoom}/{w}x{h}{highRes}{format}"), + [false, false, false, false, true, true] + ); + } + /// A schema is JSON even when the caller asked for text, and indented /// only when someone is looking at it. #[test] diff --git a/tests/output_contract.rs b/tests/output_contract.rs index 8f25d72..b534f26 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -1047,7 +1047,45 @@ fn a_missing_argument_error_names_the_arguments() { .to_string(); assert!(message.contains(""), "{message}"); - assert!(message.contains(""), "{message}"); + assert!(message.contains(""), "{message}"); +} + +/// A spec-optional positional at the end of the line can be left off, and +/// goes out the way `""` would: no placeholder and no stray separator. The +/// proxy is a dead port, so `--debug` shows the URL without it reaching +/// Mapbox. +#[test] +fn a_trailing_optional_positional_can_be_left_off() { + let out = command() + .env("HTTPS_PROXY", "http://127.0.0.1:9") + .env_remove("https_proxy") + .env_remove("ALL_PROXY") + .env_remove("all_proxy") + .env_remove("NO_PROXY") + .env_remove("no_proxy") + .args([ + "--debug", + "--token", + "pk.test", + "-u", + "mapbox", + "static", + "get-image", + "streets-v12", + "", + "24.94", + "60.17", + "12", + "0", + "0", + "600", + "400", + ]) + .output() + .expect("run mapbox"); + + let err = stderr(&out); + assert!(err.contains("/static/24.94,60.17,12,0,0/600x400?"), "{err}"); } /// A live 401, carrying the advice a caller acts on: what to do, a command From a99780db44bfa1fa4fb943e3916a9388847a98c2 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 23 Oct 2026 13:56:13 +0300 Subject: [PATCH 2/6] Let a true/false flag send false, and take snapping lists per coordinate A boolean query flag could only send true, so a parameter the API defaults to true (attribution, logo, autocomplete, dedupe, continue_straight) could never be turned off. It now also takes --flag=false; the = is required so the optional value can't swallow the next positional. A workflow step's false still leaves the flag out. directions --snapping-include-closures and --snapping-include-static-closures were switches sending a single true, which the API rejects with a 422. They now take one value per coordinate, semicolon-separated. --- CHANGELOG.md | 11 +++ .../directions/openapi/directions.yaml | 19 ++-- docs/commands.md | 17 +++- src/account_usage.rs | 7 +- src/executor.rs | 6 +- src/generate_skills.rs | 4 + src/main.rs | 12 ++- src/schema.rs | 13 ++- src/workflow/runner.rs | 20 ++-- tests/output_contract.rs | 93 +++++++++++++------ tests/schema_contract.rs | 26 ++++++ 11 files changed, 171 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ec87c1..72e951d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,17 @@ that may never merge. They are not releases and are not listed here. `` can be left off instead of passed as `""`. Writing `""` still works, so existing scripts are unaffected. `--schema` now reports these arguments as not required. +- A flag for a true/false API parameter now takes `=false`: + `--attribution=false` sends `attribution=false`. Alone it still sends + `true`, as before. Where the API defaults to `true` (`--attribution`, + `--logo`, `--autocomplete`, `--dedupe`, `--continue-straight` on driving + profiles), this is the only way to turn the setting off. A workflow + step's `false` still leaves the flag out, as before. +- `directions --snapping-include-closures` and + `--snapping-include-static-closures` take a value: `true` or `false` per + coordinate, semicolon-separated (`true;false`). They were switches that + sent a single `true`, which the API rejects with a 422 for any route, so + no working command changes. - `feedback` moved from the Search help group to Maps and data. ## 1.1.1 - 2026-10-22 diff --git a/custom-openapi/directions/openapi/directions.yaml b/custom-openapi/directions/openapi/directions.yaml index 1a1af28..dcf844a 100644 --- a/custom-openapi/directions/openapi/directions.yaml +++ b/custom-openapi/directions/openapi/directions.yaml @@ -352,22 +352,29 @@ paths: type: number minimum: 0 maximum: 100 + # One value per coordinate, not one switch: a lone `true` with two + # coordinates is a 422 ("one entry per coordinate, separated by + # ';'"), verified against production on 2026-10-23. - name: "snapping_include_closures" in: query required: false description: >- - Allow snapping to roads carrying a live-traffic closure. - `mapbox/driving-traffic` only. + Whether each coordinate may snap to a road carrying a live-traffic closure — + `true` or `false` per coordinate, semicolon-separated, with an + empty entry to skip one (`true;;false`). `mapbox/driving-traffic` + only. schema: - type: boolean + type: string - name: "snapping_include_static_closures" in: query required: false description: >- - Allow snapping to roads carrying a long-term closure. - `mapbox/driving-traffic` only. + Whether each coordinate may snap to a road carrying a long-term closure — + `true` or `false` per coordinate, semicolon-separated, with an + empty entry to skip one (`true;;false`). `mapbox/driving-traffic` + only. schema: - type: boolean + type: string - name: "walking_speed" in: query required: false diff --git a/docs/commands.md b/docs/commands.md index dbfaeee..f83a87e 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -608,6 +608,13 @@ Three things worth knowing about the read forms: `@` rather than `@-` to keep the prompt, or pass `--yes` to say the answer deliberately. +A flag that stands for a true/false API parameter, such as `--steps` or +`--attribution`, sends `true` when given alone. Write `--flag=false` to send +`false`, which matters where the API defaults to `true` (`--attribution`, +`--logo`, `--autocomplete`, `--dedupe`, `--continue-straight` on driving +profiles). The `=` is required: `--flag false` leaves `false` as a separate +argument. + A command that changes something takes `--dry-run`, which prints the request it would send, on stdout, and sends nothing. Which commands those are is not a list anyone keeps: it is every `POST`, `PUT`, `PATCH` and `DELETE` — 15 of @@ -962,7 +969,7 @@ is 2-25 `{longitude},{latitude}` pairs, semicolon-separated. | `--avoid-maneuver-radius <1-1000>` | Meters around the start to avoid a significant maneuver within. | | `--bearings ` | Filter road segments by direction of travel, one entry per coordinate. | | `--layers ` | A road layer (Z-order) per waypoint, for multi-level roads. | -| `--continue-straight` | Keep going straight at an intermediate waypoint rather than u-turning back to it. | +| `--continue-straight` | Keep going straight at an intermediate waypoint rather than u-turning back to it. Already the default on `mapbox/driving` and `mapbox/driving-traffic`; `--continue-straight=false` turns it off there. | | `--exclude ` | Road types or `point(lon lat)` values to route around, comma-separated (`motorway`, `toll`, `ferry`, `unpaved`, `cash_only_tolls`, `country_border`, `state_border`, `tunnel`). | | `--geometries ` | Route geometry format. Defaults to `polyline`. | | `--include ` | Special road types to allow, comma-separated (`hov2`, `hov3`, `hot`). | @@ -984,7 +991,7 @@ is 2-25 `{longitude},{latitude}` pairs, semicolon-separated. | `--arrive-by ` | `mapbox/driving` only. | | `--depart-at ` | `mapbox/driving` and `mapbox/driving-traffic`. | | `--max-height <0-10>` / `--max-width <0-10>` / `--max-weight <0-100>` | `mapbox/driving` and `mapbox/driving-traffic`. Meters, meters, metric tons. | -| `--snapping-include-closures` / `--snapping-include-static-closures` | `mapbox/driving-traffic` only. | +| `--snapping-include-closures ` / `--snapping-include-static-closures ` | `true` or `false` per coordinate, semicolon-separated; an empty entry skips one. `mapbox/driving-traffic` only. | | `--walking-speed <0.14-6.94>` / `--walkway-bias <-1..1>` | `mapbox/walking` only. | #### Examples @@ -1432,7 +1439,7 @@ geographic context and coordinates. | `--proximity ` | Bias results towards a point. | | `--bbox ` | Restrict to a box. | | `--language ` | IETF language tag. | -| `--autocomplete` | Partial-input matching. | +| `--autocomplete` | Partial-input matching. On by default; `--autocomplete=false` turns it off. | | `--worldview ` | Which country's view of disputed borders. | | `--permanent` | The result may be stored. Billed differently. | | `--format ` | `geojson` (default), or `v5` for the older response shape. | @@ -2720,7 +2727,7 @@ both as numbers — pass `0 0` rather than `"" ""`. | Parameter | Effect | | --- | --- | -| `--attribution` / `--logo` | Keep or drop the Mapbox attribution and logo. | +| `--attribution` / `--logo` | Both on by default; `--attribution=false` / `--logo=false` drop them. | | `--addlayer ` | Add one layer on top of the style. | | `--before-layer ` | Where to insert it. | | `--setfilter ` / `--layer-id ` | Filter an existing layer. | @@ -3413,7 +3420,7 @@ the point. | `--limit ` | How many features. Default 5, max 50. | | `--layers ` | Only these source layers. | | `--geometry ` | Only `point`, `linestring` or `polygon`. | -| `--dedupe` | Drop duplicate features. | +| `--dedupe` | Drop duplicate features. On by default; `--dedupe=false` keeps them. | | `--bands ` | Raster-array bands to sample. | | `--language` / `--worldview` | As in geocoding. | diff --git a/src/account_usage.rs b/src/account_usage.rs index 1566333..f5236af 100644 --- a/src/account_usage.rs +++ b/src/account_usage.rs @@ -661,10 +661,9 @@ fn fetch( // out of the request instead. The pinning test below is the actual // guard against that drift. // - // Mirrors `executor::dispatch`'s `is_boolean` branch: today's three - // parameters are all strings, but a future boolean one must not silently - // vanish from the request the way a bare `get_one::` would leave - // it. + // Today's three parameters are all strings, but a future boolean one + // would be a hand-declared switch in `command()`, which a bare + // `get_one::` would silently drop from the request. for param in &operation().query_params { if param.is_boolean { if matches.get_flag(¶m.arg_name) { diff --git a/src/executor.rs b/src/executor.rs index edc60ab..482d310 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -200,11 +200,7 @@ fn dispatch( } for param in &op.query_params { - if param.is_boolean { - if matches.get_flag(¶m.arg_name) { - query.push((param.name.clone(), "true".to_string())); - } - } else if let Some(val) = matches.get_one::(¶m.arg_name) { + if let Some(val) = matches.get_one::(¶m.arg_name) { query.push((param.name.clone(), val.clone())); } } diff --git a/src/generate_skills.rs b/src/generate_skills.rs index dd659a1..70d9f6e 100644 --- a/src/generate_skills.rs +++ b/src/generate_skills.rs @@ -935,6 +935,9 @@ fn describe(arg: &Argument) -> String { if arg.omit_with_empty { parts.push("Pass an empty string to leave it out of the URL.".to_string()); } + if arg.accepts_false { + parts.push(format!("Write `{}=false` to send false.", spelling(arg))); + } if !arg.conflicts_with.is_empty() { let others: Vec = arg .conflicts_with @@ -1057,6 +1060,7 @@ fn clone_argument(arg: &Argument) -> Argument { required_one_of: arg.required_one_of.clone(), fills: arg.fills.clone(), omit_with_empty: arg.omit_with_empty, + accepts_false: arg.accepts_false, env: arg.env.clone(), default: arg.default.clone(), } diff --git a/src/main.rs b/src/main.rs index 5e085fc..133fb5b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -286,9 +286,19 @@ fn build_operation_command(op: &spec::Operation) -> Command { for param in &op.query_params { let arg = if param.is_boolean { + // Present means `true`, as before, and `--flag=false` sends + // `false`: several of these default to true at the API + // (`attribution`, `autocomplete`, `dedupe`), so a bare switch + // could only ever send the default. The `=` is required so the + // optional value cannot take the next positional for itself. Arg::new(param.arg_name.clone()) .long(param.arg_name.clone()) - .action(ArgAction::SetTrue) + .num_args(0..=1) + .require_equals(true) + .default_missing_value("true") + .value_name("true|false") + .value_parser(PossibleValuesParser::new(["true", "false"])) + .hide_possible_values(true) .help(help_text(param)) } else { let mut arg = Arg::new(param.arg_name.clone()) diff --git a/src/schema.rs b/src/schema.rs index 5a4163a..741f761 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -186,11 +186,16 @@ pub(crate) struct Argument { pub(crate) fills: Option, /// Whether passing an empty string leaves this positional out of the /// URL — usually a suffix (`.png` off the end of a tile name), sometimes - /// a whole segment. The argument must still be written: clap requires - /// every path positional, and the spec's optional ones are omitted by - /// value, not by absence. + /// a whole segment. Only the run of these at the end of the line may be + /// left off altogether (`required: false`); one before a required + /// positional still has to be written, as `""`. #[serde(skip_serializing_if = "is_false")] pub(crate) omit_with_empty: bool, + /// Whether this flag also takes `--name=false`. Present alone it sends + /// `true`; the spelling with `=false` is the only way to send `false`, + /// which matters where the API defaults to `true`. + #[serde(skip_serializing_if = "is_false")] + pub(crate) accepts_false: bool, #[serde(skip_serializing_if = "Option::is_none")] pub(crate) env: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -223,6 +228,7 @@ impl Argument { required_one_of: vec![], fills: None, omit_with_empty: false, + accepts_false: false, env: None, default: None, } @@ -483,6 +489,7 @@ fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry flag: Some(format!("--{}", param.arg_name)), required: param.required, location: Some("query"), + accepts_false: param.is_boolean, ..parameter(param, kind) } })); diff --git a/src/workflow/runner.rs b/src/workflow/runner.rs index 2115995..b566afa 100644 --- a/src/workflow/runner.rs +++ b/src/workflow/runner.rs @@ -678,13 +678,19 @@ fn find_arg<'a>( }) .ok_or_else(|| format!("`{key}` is not an argument of this command"))?; let repeatable = matches!(arg.get_action(), ArgAction::Append); - let shape = if matches!(arg.get_action(), ArgAction::SetTrue | ArgAction::SetFalse) { - ArgShape::Flag - } else if arg.is_positional() { - ArgShape::Positional { repeatable } - } else { - ArgShape::Option { repeatable } - }; + // A generated boolean flag also takes `=false`, but a step's `false` + // still leaves it out: an API that reads presence rather than value + // would take `x=false` as on, and leaving it out is what `false` has + // always meant here. + let takes_false = arg.is_require_equals_set() && arg.get_num_args() == Some((0..=1).into()); + let shape = + if takes_false || matches!(arg.get_action(), ArgAction::SetTrue | ArgAction::SetFalse) { + ArgShape::Flag + } else if arg.is_positional() { + ArgShape::Positional { repeatable } + } else { + ArgShape::Option { repeatable } + }; Ok((arg, shape)) } diff --git a/tests/output_contract.rs b/tests/output_contract.rs index b534f26..3afed02 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -1050,12 +1050,9 @@ fn a_missing_argument_error_names_the_arguments() { assert!(message.contains(""), "{message}"); } -/// A spec-optional positional at the end of the line can be left off, and -/// goes out the way `""` would: no placeholder and no stray separator. The -/// proxy is a dead port, so `--debug` shows the URL without it reaching -/// Mapbox. -#[test] -fn a_trailing_optional_positional_can_be_left_off() { +/// The URL a command would request, read off `--debug`. The proxy is a dead +/// port, so nothing reaches Mapbox. +fn debug_url(args: &[&str]) -> String { let out = command() .env("HTTPS_PROXY", "http://127.0.0.1:9") .env_remove("https_proxy") @@ -1063,29 +1060,73 @@ fn a_trailing_optional_positional_can_be_left_off() { .env_remove("all_proxy") .env_remove("NO_PROXY") .env_remove("no_proxy") - .args([ - "--debug", - "--token", - "pk.test", - "-u", - "mapbox", - "static", - "get-image", - "streets-v12", - "", - "24.94", - "60.17", - "12", - "0", - "0", - "600", - "400", - ]) + .args(["--debug", "--token", "pk.test", "-u", "mapbox"]) + .args(args) .output() .expect("run mapbox"); - let err = stderr(&out); - assert!(err.contains("/static/24.94,60.17,12,0,0/600x400?"), "{err}"); + err.lines() + .find_map(|line| line.strip_prefix("[debug] GET ")) + .unwrap_or_else(|| panic!("no request in {err}")) + .to_string() +} + +const STATIC_IMAGE: &[&str] = &[ + "static", + "get-image", + "streets-v12", + "", + "24.94", + "60.17", + "12", + "0", + "0", + "600", + "400", +]; + +/// A spec-optional positional at the end of the line can be left off, and +/// goes out the way `""` would: no placeholder and no stray separator. +#[test] +fn a_trailing_optional_positional_can_be_left_off() { + let url = debug_url(STATIC_IMAGE); + assert!(url.contains("/static/24.94,60.17,12,0,0/600x400?"), "{url}"); +} + +/// `attribution` and `logo` default to true at the API, so a flag that could +/// only send `true` could never take them off the image. +#[test] +fn a_boolean_flag_sends_false_when_told_to() { + let url = debug_url(&[STATIC_IMAGE, &["--attribution=false", "--logo"]].concat()); + assert!(url.contains("attribution=false"), "{url}"); + assert!(url.contains("logo=true"), "{url}"); +} + +/// With the `=` required, `false` after a space is not the flag's value, so +/// the optional value cannot swallow the next positional either. +#[test] +fn a_boolean_flag_value_needs_the_equals_sign() { + let out = command() + .args(["geocoder", "forward", "--q", "x", "--autocomplete", "false"]) + .output() + .expect("run mapbox"); + assert_eq!(out.status.code(), Some(2), "{}", stderr(&out)); +} + +/// One value per coordinate: a single switch is a 422 at the API. +#[test] +fn directions_snapping_takes_a_list_per_coordinate() { + let url = debug_url(&[ + "directions", + "mapbox/driving-traffic", + "24.94,60.17;24.95,60.18", + "--snapping-include-closures", + "true;true", + ]); + assert!( + url.contains("snapping_include_closures=true%3Btrue"), + "{url}" + ); } /// A live 401, carrying the advice a caller acts on: what to do, a command diff --git a/tests/schema_contract.rs b/tests/schema_contract.rs index 4be552c..771751f 100644 --- a/tests/schema_contract.rs +++ b/tests/schema_contract.rs @@ -120,6 +120,32 @@ fn a_command_describes_itself_without_being_able_to_run() { assert_eq!(style_id["location"], "path"); } +/// What an agent needs to write `static get-image` without guessing: which +/// positionals it may leave off, and that a boolean flag also takes `=false`. +#[test] +fn optional_positionals_and_false_flags_are_described() { + let value = schema(&["static", "get-image", "--schema"]); + let arguments = commands(&value)[0]["arguments"] + .as_array() + .expect("arguments"); + let arg = |name: &str| { + arguments + .iter() + .find(|arg| arg["name"] == name) + .unwrap_or_else(|| panic!("`{name}` is described")) + }; + + // Last on the line, so it can go. + assert_eq!(arg("format")["required"], false); + assert_eq!(arg("format")["omit_with_empty"], true); + // Before a required positional, so `""` has to be written. + assert_eq!(arg("overlay")["required"], true); + assert_eq!(arg("overlay")["omit_with_empty"], true); + + assert_eq!(arg("attribution")["kind"], "flag"); + assert_eq!(arg("attribution")["accepts_false"], true); +} + #[test] fn the_account_the_url_needs_is_listed_as_an_argument() { // `{username}` has no parameter of its own — the executor fills it from From 8826f69f02f392ef8383a2a5d94448f49f0a3633 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 23 Oct 2026 13:56:21 +0300 Subject: [PATCH 3/6] Show whole descriptions in --help, as plain text A parameter's help was cut to its first sentence, which dropped the allowed values and what else it needs ("Requires overview=full"). It now shows the whole description. An API command's or service's --help printed the spec's Markdown page as is: headings, tables, curl examples. It now shows the opening prose, rendered as plain text, then a Docs: link. --schema still carries the full description. --- CHANGELOG.md | 7 + Cargo.lock | 12 ++ Cargo.toml | 6 + .../directions/openapi/directions.yaml | 6 - custom-openapi/feedback/openapi/feedback.yaml | 4 - .../isochrone/openapi/isochrone.yaml | 8 - src/main.rs | 91 +++++++-- src/markdown.rs | 174 ++++++++++++++++++ src/spec.rs | 9 +- tests/output_contract.rs | 46 +++++ 10 files changed, 326 insertions(+), 37 deletions(-) create mode 100644 src/markdown.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 72e951d..0865c8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,13 @@ that may never merge. They are not releases and are not listed here. coordinate, semicolon-separated (`true;false`). They were switches that sent a single `true`, which the API rejects with a 422 for any route, so no working command changes. +- `--help` shows each parameter's whole description rather than its first + sentence, so the allowed values and what else a parameter needs + ("Requires `overview=full`") are no longer cut off. +- An API command's `--help` shows the opening of its description as plain + text, followed by a `Docs:` link, instead of the whole Markdown page with + its headings, tables and curl examples (`geocoder batch` was 113 lines). + `--schema` still carries the full description. - `feedback` moved from the Search help group to Maps and data. ## 1.1.1 - 2026-10-22 diff --git a/Cargo.lock b/Cargo.lock index c19a4a9..cf52f9b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -828,6 +828,7 @@ dependencies = [ "jsonc-parser", "nix", "open", + "pulldown-cmark", "rand", "reqwest", "serde", @@ -975,6 +976,17 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "pulldown-cmark" +version = "0.13.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "278e9268c93b9e498a2da46c34d90ba93a3255eb99009646398f34bc73ab36eb" +dependencies = [ + "bitflags", + "memchr", + "unicase", +] + [[package]] name = "quinn" version = "0.11.12" diff --git a/Cargo.toml b/Cargo.toml index 0654710..7592fc1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,6 +119,12 @@ terminal-colorsaurus = "1" # a signal handler takes `unsafe` this crate does not have. Installed by # that command only; every other command keeps the default. ctrlc = { version = "3", features = ["termination"] } +# Reads the specs' Markdown descriptions for `--help`: the opening prose +# only, with its inline markup rendered rather than printed. A parser rather +# than pattern matching, because stripping markers by hand is what turned +# the code spans `and` and `x` into "and or x". Default features off: they +# are the HTML renderer and a command-line binary. +pulldown-cmark = { version = "0.13", default-features = false } [target.'cfg(unix)'.dependencies] # For `workflow run`: passing a SIGTERM on to the running step, which a diff --git a/custom-openapi/directions/openapi/directions.yaml b/custom-openapi/directions/openapi/directions.yaml index dcf844a..6265012 100644 --- a/custom-openapi/directions/openapi/directions.yaml +++ b/custom-openapi/directions/openapi/directions.yaml @@ -292,12 +292,6 @@ paths: - name: "alley_bias" in: query required: false - # Content before the profile note, not after: `first_sentence` in - # `src/main.rs` cuts a `--help` line at the first `.`, and - # `` `mapbox/driving` only. `` — a real sentence on its own — was - # eating everything that followed it. `--schema` and - # `docs/commands.md` show the description in full either way — - # this ordering is only for the one-line case. description: >- Bias the route against (negative) or toward (positive) alleys, -1 to 1. `mapbox/driving` only. diff --git a/custom-openapi/feedback/openapi/feedback.yaml b/custom-openapi/feedback/openapi/feedback.yaml index 4c1b6de..42af12b 100644 --- a/custom-openapi/feedback/openapi/feedback.yaml +++ b/custom-openapi/feedback/openapi/feedback.yaml @@ -111,10 +111,6 @@ paths: the feedback. schema: type: string - # "ISO 8601" trails each of these six rather than leads them: - # `first_sentence` in `src/main.rs` cuts a `--help` line at the - # first `.`, and "ISO 8601." on its own left `--help` showing just - # that. `--schema` and `docs/commands.md` still show each in full. - name: "created_before" in: query required: false diff --git a/custom-openapi/isochrone/openapi/isochrone.yaml b/custom-openapi/isochrone/openapi/isochrone.yaml index 9aff3fa..4296f9a 100644 --- a/custom-openapi/isochrone/openapi/isochrone.yaml +++ b/custom-openapi/isochrone/openapi/isochrone.yaml @@ -113,14 +113,6 @@ paths: - name: "denoise" in: query required: false - # Spelled "0 to 1"/"1", not "0.0-1.0"/"1.0": `first_sentence` in - # `src/main.rs` cuts a `--help` line at the first `.`, and this - # value's own bounds are the first digits that would appear no - # matter where in the sentence they sit, so a literal decimal - # point anywhere before the end truncates it — not just at the - # front, the way `directions.yaml`'s own fix assumed. `--schema` - # and `docs/commands.md` still show the fuller description below - # in full, decimals included. description: >- A larger value removes more of the smaller contours, 0 to 1, defaulting to 1. A value of 1 returns only the largest contour diff --git a/src/main.rs b/src/main.rs index 133fb5b..1fb01a0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -40,6 +40,7 @@ mod help_layout; mod history; mod http; mod link; +mod markdown; mod mcp; mod output; mod remedy; @@ -129,11 +130,8 @@ fn looks_like_a_flag(raw: &str) -> bool { } } -/// A parameter's description, cut to its first sentence for a line of help. -/// -/// The spec text is kept whole by `parse_spec` — `--schema` publishes it and -/// a cut there lands inside identifiers and decimals. Help has a line to -/// work with, so it shortens here, at the only place that wants it. +/// A description cut to its first sentence, for a table cell that has one +/// line to work with. /// /// A sentence ends at a full stop followed by white space, or at the end. /// Cutting at any `.` stopped help inside `{owner}.{tileset}`, `-85.0511` @@ -156,13 +154,38 @@ fn ends_with_abbreviation(text: &str) -> bool { ["e.g.", "i.e."].iter().any(|abbr| text.ends_with(abbr)) } +/// A spec description followed by the page that has the rest of it: help +/// keeps only the opening prose (see [`markdown::intro`]). +fn with_docs_link(text: String, service: &str) -> String { + match remedy::docs_for_service(service) { + Some(url) => format!("{text}\n\nDocs: {url}"), + None => text, + } +} + +/// A parameter's whole description, as one line of help. +/// +/// Whole rather than its first sentence: the second is where a spec says +/// which values are allowed and what else the parameter needs ("Requires +/// `overview=full`", "Required, here or in `--data`"), and help cut there +/// left a caller to find that out from the API's error. The spec's own line +/// breaks are folded, since clap wraps the text itself. fn help_text(param: &spec::Parameter) -> String { param .description .as_deref() - .map(first_sentence) + .map(help_line) .unwrap_or_default() - .to_string() +} + +/// Rendered, and without the closing full stop that clap's own help lines +/// leave off. +fn help_line(text: &str) -> String { + let text = markdown::render(text); + match text.strip_suffix('.') { + Some(stripped) => stripped.to_string(), + None => text, + } } /// The API's value for a spec-optional path parameter that clap cannot take @@ -251,6 +274,7 @@ fn build_operation_command(op: &spec::Operation) -> Command { } if let Some(desc) = &op.description { + let desc = with_docs_link(markdown::render(markdown::intro(desc)), &op.service); // Marked here too, not only in `about`. Clap renders `long_about` // for `--help` and `about` for `-h`, so a command with a spec // description — all 54 of them have one — showed the marker in its @@ -259,7 +283,7 @@ fn build_operation_command(op: &spec::Operation) -> Command { cmd = cmd.long_about(if op.deprecated { format!("[deprecated] {desc}") } else { - desc.clone() + desc }); } @@ -417,9 +441,8 @@ fn build_operation_command(op: &spec::Operation) -> Command { let help = variadic .description .as_deref() - .map(first_sentence) - .unwrap_or_default() - .to_string(); + .map(help_line) + .unwrap_or_default(); cmd = cmd.arg( Arg::new(variadic.arg_name.clone()) .required(true) @@ -532,7 +555,7 @@ fn build_service_command(svc: &ServiceSpec) -> Command { .subcommand_required(true); if let Some(desc) = &svc.description { - cmd = cmd.long_about(desc.clone()); + cmd = cmd.long_about(with_docs_link(markdown::render(desc), &svc.name)); } attach_operations(cmd, &svc.name, &exposed, 0) @@ -2358,7 +2381,17 @@ mod tests { /// keeps the paragraph, and this is where it gets cut back — and the /// sentence ends at a full stop, not at a dot inside an id or a number. #[test] - fn help_shortens_a_description_to_its_first_sentence() { + fn help_is_the_whole_description_on_one_line() { + assert_eq!( + help_line("A layer to add to the map\nbefore rendering. Maximum one per request."), + "A layer to add to the map before rendering. Maximum one per request" + ); + assert_eq!(help_line(" no full stop here "), "no full stop here"); + assert_eq!(help_line(""), ""); + } + + #[test] + fn first_sentence_stops_at_a_real_full_stop() { assert_eq!( first_sentence("Tileset ID in the format `username.id`. Order matters."), "Tileset ID in the format `username.id`" @@ -2561,6 +2594,38 @@ mod tests { assert_eq!(seen, expected, "auth lost a subcommand"); } + /// The specs' descriptions are Markdown written for docs.mapbox.com, and + /// none of that markup may reach `--help`: no heading, table, fence, + /// ``, bold marker or bare link syntax, on any command or service. + #[test] + fn no_markdown_reaches_help() { + let specs = bundled_specs(); + let parsing = help_layout::for_parsing(help_layout::apply(build_app(&specs))); + let mut paths: Vec> = leaf_commands(&parsing, &[]) + .into_iter() + .map(|(path, _)| path) + .collect(); + paths.extend(specs.iter().map(|svc| vec![svc.name.clone()])); + + let mut problems = vec![]; + for path in paths { + let argv: Vec<&str> = std::iter::once("mapbox") + .chain(path.iter().map(String::as_str)) + .chain(["--help"]) + .collect(); + let Err(help) = parsing.clone().try_get_matches_from(&argv) else { + continue; + }; + let help = help.render().to_string(); + for marker in ["## ", "**", "```", "` blocks around the prose. Printed as they are, that +//! made `geocoder batch --help` 113 lines of markup, most of it about calling +//! the API by hand. [`intro`] keeps the opening prose, and [`render`] turns +//! its markup into plain text. +//! +//! Only spec text comes through here. Hand-written help is plain text with +//! its own line breaks, which Markdown would fold into one paragraph. +//! `--schema` keeps the source: an agent reads Markdown well and wants it +//! whole. + +use pulldown_cmark::{Event, Options, Parser, Tag, TagEnd}; + +/// The opening prose of `text`, as Markdown: paragraphs and lists up to the +/// first heading, code block, table, quote, rule or HTML block. What follows +/// one of those is reference material — option tables, worked examples, +/// billing notes — that the page linked from help covers in full. +pub fn intro(text: &str) -> &str { + let mut depth = 0usize; + let mut end = 0; + for (event, range) in Parser::new_ext(text, Options::ENABLE_TABLES).into_offset_iter() { + match event { + Event::Start(tag) => { + if depth == 0 && !matches!(tag, Tag::Paragraph | Tag::List(_)) { + break; + } + depth += 1; + } + Event::End(_) => { + depth -= 1; + if depth == 0 { + end = range.end; + } + } + Event::Html(_) | Event::Rule if depth == 0 => break, + _ => {} + } + } + text[..end].trim_end() +} + +/// `text` with its Markdown rendered as plain text. +/// +/// Plain rather than styled: clap's wrapping doubles every line break in a +/// paragraph that opens with a styled run right after a blank line (clap +/// 4.6), which is how `tilesets get-mvt`'s "**Style-optimized tiles**:" +/// paragraph came out. Markers are dropped, a link keeps its address in +/// parentheses since a terminal may not make it clickable, paragraphs are +/// separated by a blank line and list items start their own line. +pub fn render(text: &str) -> String { + let mut out = String::new(); + let mut lists: Vec> = vec![]; + let mut links: Vec = vec![]; + let mut link_text_start = 0; + + for event in Parser::new_ext(text, Options::ENABLE_TABLES) { + match event { + Event::Start(Tag::Paragraph) | Event::Start(Tag::Heading { .. }) + if lists.is_empty() => + { + separate(&mut out, "\n\n"); + } + Event::End(TagEnd::Heading(_)) => out.push('\n'), + Event::Start(Tag::List(first)) => { + if lists.is_empty() { + separate(&mut out, "\n"); + } + lists.push(first); + } + Event::End(TagEnd::List(_)) => { + lists.pop(); + } + Event::Start(Tag::Item) => { + separate(&mut out, "\n"); + out.push_str(&" ".repeat(lists.len().saturating_sub(1))); + match lists.last_mut() { + Some(Some(n)) => { + out.push_str(&format!("{n}. ")); + *n += 1; + } + _ => out.push_str("- "), + } + } + Event::Code(code) => out.push_str(&code), + Event::Start(Tag::Link { dest_url, .. }) => { + links.push(dest_url.to_string()); + link_text_start = out.len(); + } + Event::End(TagEnd::Link) => { + if let Some(url) = links.pop() { + if out[link_text_start..] != url { + out.push_str(&format!(" ({url})")); + } + } + } + Event::Start(Tag::CodeBlock(_)) => separate(&mut out, "\n\n"), + Event::Text(text) => out.push_str(&text), + Event::SoftBreak => out.push(' '), + Event::HardBreak => out.push('\n'), + _ => {} + } + } + out.trim().to_string() +} + +/// Ends what came before with `gap` — unless nothing did, or it already ends +/// that way. +fn separate(out: &mut String, gap: &str) { + let trimmed = out.trim_end_matches(' ').len(); + out.truncate(trimmed); + if out.is_empty() || out.ends_with(gap) { + return; + } + while out.ends_with('\n') { + out.pop(); + } + out.push_str(gap); +} + +#[cfg(test)] +mod tests { + use super::*; + + fn plain(text: &str) -> String { + render(text) + } + + #[test] + fn the_intro_stops_at_the_first_reference_block() { + let text = "Looks up a place.\n\n## Structured input\n\nMore."; + assert_eq!(intro(text), "Looks up a place."); + + let text = "Up to 50 queries.\n\n\n Each counts.\n\n"; + assert_eq!(intro(text), "Up to 50 queries."); + + let text = "Run it:\n\n```bash\ncurl …\n```\n"; + assert_eq!(intro(text), "Run it:"); + } + + /// A paragraph that ends in a colon keeps the list it introduces. + #[test] + fn the_intro_keeps_a_list() { + let text = "Three kinds:\n\n - **Public** (`pk`)\n - **Secret** (`sk`)\n\n| a | b |\n|---|---|\n| 1 | 2 |\n"; + assert_eq!( + intro(text), + "Three kinds:\n\n - **Public** (`pk`)\n - **Secret** (`sk`)" + ); + } + + #[test] + fn inline_markup_is_rendered_rather_than_printed() { + assert_eq!( + plain("The **Geocoding API** does _two_ things\nwith `q`."), + "The Geocoding API does two things with q." + ); + assert_eq!( + plain("See [Slippy Map Tilenames](https://wiki.example/x)."), + "See Slippy Map Tilenames (https://wiki.example/x)." + ); + // The code spans a regex stripped into "and or x". + assert_eq!(plain("`and` or `x`"), "and or x"); + } + + #[test] + fn paragraphs_and_lists_keep_their_shape() { + assert_eq!( + plain("Two things:\n\n- Forward\n- Reverse\n\nAfter."), + "Two things:\n- Forward\n- Reverse\n\nAfter." + ); + assert_eq!(plain("Steps:\n\n1. One\n2. Two"), "Steps:\n1. One\n2. Two"); + } +} diff --git a/src/spec.rs b/src/spec.rs index ec9e189..57836c7 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -1086,12 +1086,11 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result { .unwrap_or(service_name) .to_string(); - // Flattened to one line (unlike a parameter's description): this - // renders as a service's `long_about`, where the spec's own line - // wrapping doesn't help. + // The opening prose, as Markdown: a first paragraph alone stopped + // `accounts` at "There are three kinds of tokens:", before the list. let description = doc["info"]["description"] .as_str() - .map(|text| first_paragraph(text).replace('\n', " ")); + .map(|text| crate::markdown::intro(text).to_string()); let base_url = doc["servers"] .as_sequence() @@ -1669,8 +1668,6 @@ fn parse_parameter(val: &Value) -> Option { // extreme: `geocoder --types` alone is 4.6 KB of feature-type // reference, and the schema gets read on every call. A paragraph // avoids both problems, and leaves the appendices out. - // `crate::first_sentence` shortens it further for `--help`, which - // still renders exactly as it always has. let description = val["description"].as_str().map(first_paragraph); let schema = &val["schema"]; diff --git a/tests/output_contract.rs b/tests/output_contract.rs index 3afed02..83554a2 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -1050,6 +1050,52 @@ fn a_missing_argument_error_names_the_arguments() { assert!(message.contains(""), "{message}"); } +/// A parameter's help is its whole description: the sentences after the +/// first are where a spec lists the values and what else the parameter +/// needs. Whitespace is folded, since clap wraps the text at its own width. +#[test] +fn help_shows_a_parameter_s_whole_description() { + let help = |args: &[&str]| { + let out = run(args); + assert!(out.status.success(), "{args:?}: {}", stderr(&out)); + stdout(&out) + .split_whitespace() + .collect::>() + .join(" ") + }; + + let matrix = help(&["matrix", "--help"]); + assert!( + matrix.contains("Options are duration (the default) and distance"), + "{matrix}" + ); + let directions = help(&["directions", "--help"]); + assert!( + directions.contains("Requires overview=full"), + "{directions}" + ); + let feedback = help(&["feedback", "create", "--help"]); + assert!( + feedback.contains("Required, here or in --data"), + "{feedback}" + ); +} + +/// A spec's description stops at its reference material, keeps a list its +/// opening introduces, and ends with the page that has the rest. +#[test] +fn help_keeps_a_spec_description_s_opening_prose() { + let forward = stdout(&run(&["geocoder", "forward", "--help"])); + assert!(!forward.contains("Structured Input"), "{forward}"); + assert!( + forward.contains("Docs: https://docs.mapbox.com/api/search/geocoding-v6/"), + "{forward}" + ); + + let accounts = stdout(&run(&["accounts", "--help"])); + assert!(accounts.contains("- Temporary tokens (tk)"), "{accounts}"); +} + /// The URL a command would request, read off `--debug`. The proxy is a dead /// port, so nothing reaches Mapbox. fn debug_url(args: &[&str]) -> String { From 873cb63739e5d4314ab7669c9dd0ed2f012501d6 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 23 Oct 2026 13:56:29 +0300 Subject: [PATCH 4/6] Report hand-read env and defaults in --schema, add groups, refuse a stray --id --timeout, --output, --no-color, --profile and workflow install --repo/--ref read their variable or default in code, so --schema only had them in the description. A [env: X] or [default: X] note in the help now fills the env and default fields. A new top-level groups list carries what a command group's own help says, which commands (leaves only) never included. --id is global but only API commands read it, so history show --id x printed the newest run as if it were x. Anywhere else it is now a usage error. --- CHANGELOG.md | 10 +++ docs/commands.md | 2 +- src/main.rs | 21 +++++- src/schema.rs | 139 +++++++++++++++++++++++++++++++++++++-- tests/output_contract.rs | 15 +++++ tests/schema_contract.rs | 56 ++++++++++++++++ 6 files changed, 234 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0865c8f..4badb95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,16 @@ that may never merge. They are not releases and are not listed here. text, followed by a `Docs:` link, instead of the whole Markdown page with its headings, tables and curl examples (`geocoder batch` was 113 lines). `--schema` still carries the full description. +- `--id` on a command that is not an API command (`history show`, + `auth profiles`, `config list` and the like) is now a usage error, exit + code 2. It used to be ignored: `history show --id x` printed the newest + run as if it were `x`. +- `--schema`: `env` and `default` are filled for every argument, including + the ones read by hand (`--timeout`, `--output`, `--no-color`, `--profile`, + `workflow install --repo`/`--ref`), which only mentioned them in their + description. A new top-level `groups` list carries what a command group's + own help says, such as `mapbox history`, which `commands` (leaves only) + never included. - `feedback` moved from the Search help group to Maps and data. ## 1.1.1 - 2026-10-22 diff --git a/docs/commands.md b/docs/commands.md index f83a87e..639b21e 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -566,7 +566,7 @@ either. | `--use-login` | Ignore `MAPBOX_ACCESS_TOKEN`; use stored credentials. | | `--profile ` | Which stored credentials to use. | | `--output`, `-o` | `auto` \| `text` \| `json`. | -| `--id ` | On a command that returns a list, print just the row with that `id` or `name`. | +| `--id ` | On an API command that returns a list, print just the row with that `id` or `name`. Any other command rejects it. | | `--quiet`, `-q` | Don't print the `mapbox · v` banner, which goes to stderr and only when stderr is a terminal, the note after a download, or a workflow step's details. Also `MAPBOX_QUIET`. | | `--no-color` | Turn off color in help, errors and text output. Same as `NO_COLOR`; wins over `FORCE_COLOR`. | | `--timeout ` | How long one request may take, connection included. Defaults to 60 seconds, or 900 for a body read from `--file` or from a `--data @`/`@-`. Also `MAPBOX_TIMEOUT`. | diff --git a/src/main.rs b/src/main.rs index 1fb01a0..a2a12e7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -605,7 +605,7 @@ fn build_app(specs: &[ServiceSpec]) -> Command { .value_name("PROFILE") .help_heading(AUTH_OPTIONS) .global(true) - .help("Credential profile to use (default: \"default\")") + .help("Credential profile to use [default: default]") .required(false), ) .arg( @@ -695,7 +695,7 @@ fn build_app(specs: &[ServiceSpec]) -> Command { .value_name("ID") .help_heading(OUTPUT_OPTIONS) .global(true) - .help("Show only the row with this id from a list"), + .help("Show only the row with this id from an API listing"), ) .arg( Arg::new(output::ARG) @@ -725,7 +725,7 @@ fn build_app(specs: &[ServiceSpec]) -> Command { .action(ArgAction::SetTrue) .help_heading(OUTPUT_OPTIONS) .global(true) - .help("Turn off color, as NO_COLOR does"), + .help("Turn off color [env: NO_COLOR]"), ) .arg( Arg::new(schema::ARG) @@ -1397,6 +1397,21 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - deprecation::warn_command(&path); } + // `--id` is global so that every API command takes it, but only the + // executor reads it. Anywhere else it was ignored, and `history show + // --id x` printed the newest run as if it were `x`. + if matches.get_one::(output::FILTER_ARG).is_some() + && !matches + .subcommand_name() + .is_some_and(|name| specs.iter().any(|svc| svc.name == name)) + { + return Err(CliError::new( + "usage", + "`--id` only applies to an API command that returns a list.", + ) + .into()); + } + match matches.subcommand() { // Ahead of the generic service arm, which would otherwise catch this // as an unknown service. `tilesets` makes its own HTTP calls, so the diff --git a/src/schema.rs b/src/schema.rs index 741f761..9b8d68a 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -60,9 +60,22 @@ pub(crate) struct Schema { target: String, pub(crate) authentication: &'static str, pub(crate) global_options: Vec, + /// The commands that hold others — `mapbox history`, `mapbox geocoder` — + /// with what their own help says. Only leaves are in `commands`, and the + /// group's text is often the part that applies to all of them: how long + /// history is kept, that `config` writes a file. + pub(crate) groups: Vec, pub(crate) commands: Vec, } +#[derive(serde::Serialize)] +pub(crate) struct Group { + pub(crate) command: String, + pub(crate) summary: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) description: Option, +} + #[derive(serde::Serialize)] pub(crate) struct Cli { pub(crate) name: &'static str, @@ -317,10 +330,68 @@ pub(crate) fn build(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Sc // Whole, at every level: a caller that asked about one command still // needs to know a token goes on the front of it. global_options: global_options(app), + groups: groups(app, specs, path), commands: commands(app, specs, path), } } +/// Every group on the way to the target or under it: `mapbox history list +/// --schema` still gets `mapbox history`, whose text covers it. +fn groups(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Vec { + let mut out = vec![]; + collect_groups(app, specs, &mut vec![], path, &mut out); + out +} + +fn collect_groups( + cmd: &Command, + specs: &[ServiceSpec], + at: &mut Vec, + target: &[String], + out: &mut Vec, +) { + let children = || { + cmd.get_subcommands() + .filter(|sub| sub.get_name() != "help" && !sub.is_hide_set()) + }; + if !at.is_empty() && children().next().is_some() { + let related = at.starts_with(target) || target.starts_with(at); + if related { + out.push(group(cmd, specs, at)); + } + } + for sub in children() { + at.push(sub.get_name().to_string()); + collect_groups(sub, specs, at, target, out); + at.pop(); + } +} + +fn group(cmd: &Command, specs: &[ServiceSpec], at: &[String]) -> Group { + let service = (at.len() == 1) + .then(|| specs.iter().find(|svc| svc.name == at[0])) + .flatten(); + Group { + command: std::iter::once("mapbox") + .chain(at.iter().map(String::as_str)) + .collect::>() + .join(" "), + // An API service's `about` is its spec's title ("Mapbox Tokens + // API"); the help listing's line says what the group is for. + summary: crate::help_layout::description(&at[0]) + .filter(|_| at.len() == 1) + .map(String::from) + .or_else(|| cmd.get_about().map(ToString::to_string)) + .unwrap_or_default(), + // A service's spec Markdown rather than the help rendered from it, + // the same as an API command's `description`. + description: match service { + Some(svc) => svc.description.clone(), + None => long_about(cmd), + }, + } +} + fn global_options(app: &Command) -> Vec { app.get_arguments() .filter(|arg| arg.is_global_set()) @@ -887,6 +958,7 @@ fn long_about(cmd: &Command) -> Option { /// command declares. fn clap_argument(arg: &Arg) -> Argument { let long = arg.get_long(); + let help = arg.get_help().map(ToString::to_string); let is_flag = matches!(arg.get_action(), ArgAction::SetTrue | ArgAction::SetFalse); let kind = match (long.is_some(), is_flag) { @@ -919,9 +991,9 @@ fn clap_argument(arg: &Arg) -> Argument { .map(|value| value.get_name().to_string()) .collect() }, - description: arg - .get_help() - .map(ToString::to_string) + description: help + .as_deref() + .map(without_hand_notes) .filter(|help| !help.is_empty()), // `--file` repeats by being written again; `tilesets-cli`'s // passthrough takes many values at once. Both mean "more than one". @@ -929,12 +1001,17 @@ fn clap_argument(arg: &Arg) -> Argument { || arg .get_num_args() .is_some_and(|range| range.max_values() > 1), - env: arg.get_env().and_then(|env| env.to_str()).map(String::from), + env: arg + .get_env() + .and_then(|env| env.to_str()) + .map(String::from) + .or_else(|| hand_note(help.as_deref(), "env")), default: arg .get_default_values() .first() .and_then(|value| value.to_str()) - .map(String::from), + .map(String::from) + .or_else(|| hand_note(help.as_deref(), "default")), ..Argument::new( long.unwrap_or_else(|| arg.get_id().as_str()).to_string(), kind, @@ -942,6 +1019,39 @@ fn clap_argument(arg: &Arg) -> Argument { } } +/// A `[env: X]` or `[default: X]` written into an argument's help by hand. +/// +/// Some arguments read their variable or apply their default in code rather +/// than through clap, so that an unusable `MAPBOX_OUTPUT=` or +/// `MAPBOX_TIMEOUT=30s` warns instead of failing every command. Their help +/// spells the note the way clap would, and this reads it back, so the +/// schema's `env` and `default` hold for every argument rather than only +/// those clap knows about. +fn hand_note(help: Option<&str>, key: &str) -> Option { + let open = format!("[{key}: "); + let start = help?.find(&open)? + open.len(); + let rest = &help?[start..]; + Some(rest[..rest.find(']')?].to_string()) +} + +/// `help` without the notes [`hand_note`] reads, which the schema reports +/// as fields instead. +fn without_hand_notes(help: &str) -> String { + let mut text = help.to_string(); + for key in ["env", "default"] { + let open = format!("[{key}: "); + if let Some(start) = text.find(&open) { + if let Some(len) = text[start..].find(']') { + text.replace_range(start..=start + len, ""); + } + } + } + text.split_whitespace() + .collect::>() + .join(" ") + .replace(" .", ".") +} + #[cfg(test)] mod tests { use super::*; @@ -1393,6 +1503,25 @@ mod tests { ); } + #[test] + fn a_hand_written_note_becomes_a_field() { + let help = "GitHub repository to install from [default: mapbox/mapbox-cli]. Reads \ + GH_TOKEN for a private one"; + assert_eq!( + hand_note(Some(help), "default").as_deref(), + Some("mapbox/mapbox-cli") + ); + assert_eq!(hand_note(Some(help), "env"), None); + assert_eq!( + without_hand_notes(help), + "GitHub repository to install from. Reads GH_TOKEN for a private one" + ); + assert_eq!( + without_hand_notes("Seconds per request [env: MAPBOX_TIMEOUT]"), + "Seconds per request" + ); + } + /// A schema is JSON even when the caller asked for text, and indented /// only when someone is looking at it. #[test] diff --git a/tests/output_contract.rs b/tests/output_contract.rs index 83554a2..fb14697 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -1096,6 +1096,21 @@ fn help_keeps_a_spec_description_s_opening_prose() { assert!(accounts.contains("- Temporary tokens (tk)"), "{accounts}"); } +/// Only the executor reads `--id`, so any other command refuses it rather +/// than printing everything as if it had matched. +#[test] +fn id_is_refused_where_nothing_reads_it() { + for args in [ + &["-o", "json", "history", "show", "--id", "nope"][..], + &["-o", "json", "config", "list", "--id", "nope"], + ] { + let out = run(args); + assert_eq!(out.status.code(), Some(2), "{args:?}: {}", stderr(&out)); + assert!(stdout(&out).is_empty(), "{args:?}: {}", stdout(&out)); + assert_eq!(json(&stderr(&out))["code"], "usage", "{args:?}"); + } +} + /// The URL a command would request, read off `--debug`. The proxy is a dead /// port, so nothing reaches Mapbox. fn debug_url(args: &[&str]) -> String { diff --git a/tests/schema_contract.rs b/tests/schema_contract.rs index 771751f..9b506d7 100644 --- a/tests/schema_contract.rs +++ b/tests/schema_contract.rs @@ -120,6 +120,62 @@ fn a_command_describes_itself_without_being_able_to_run() { assert_eq!(style_id["location"], "path"); } +/// An argument that reads its variable or its default by hand still +/// reports it in `env` and `default`, and the note its help spells it with +/// is not left in the description. +#[test] +fn every_env_and_default_is_a_field() { + let value = schema(&["--schema"]); + let global = |name: &str| { + value["global_options"] + .as_array() + .expect("global options") + .iter() + .find(|arg| arg["name"] == name) + .unwrap_or_else(|| panic!("`{name}` is described")) + .clone() + }; + assert_eq!(global("timeout")["env"], "MAPBOX_TIMEOUT"); + assert_eq!(global("output")["env"], "MAPBOX_OUTPUT"); + assert_eq!(global("no-color")["env"], "NO_COLOR"); + assert_eq!(global("profile")["default"], "default"); + + let arguments = value["global_options"] + .as_array() + .into_iter() + .flatten() + .chain( + commands(&value) + .iter() + .flat_map(|command| command["arguments"].as_array().into_iter().flatten()), + ); + for arg in arguments { + let description = arg["description"].as_str().unwrap_or_default(); + assert!( + !description.contains("[env: ") && !description.contains("[default: "), + "`{}` keeps a note the schema should report as a field: {description}", + arg["name"] + ); + } +} + +/// Only leaves are commands, so what a group's own help says lives in +/// `groups` — including for a target below it. +#[test] +fn a_group_s_own_description_is_described() { + let value = schema(&["history", "list", "--schema"]); + let groups = value["groups"].as_array().expect("groups"); + assert_eq!(groups.len(), 1, "{groups:?}"); + assert_eq!(groups[0]["command"], "mapbox history"); + assert!( + groups[0]["description"] + .as_str() + .unwrap_or_default() + .contains("kept for 30 days"), + "{groups:?}" + ); +} + /// What an agent needs to write `static get-image` without guessing: which /// positionals it may leave off, and that a boolean flag also takes `=false`. #[test] From a6c95a69961c4b3d60aba08d652ef4628dc25353 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 23 Oct 2026 13:56:36 +0300 Subject: [PATCH 5/6] Fix help wording that was wrong, vague or written for the API Hand-written commands: skill --agent/--dir/--ref, mcp list/install, uninstall (names what stays in ~/.mapbox), config set's keys and defaults, and usage's long help, which no longer points at the issue tracker. custom-openapi: routing profiles, matrix sources/destinations, directions --waypoints, map-match --approaches, feedback list's pagination and the places description. The feedback feature and screenshot notes record what the API accepted on 2026-10-23. docs.tsv now ranks 23 queries first, up from 22. --- .../directions/openapi/directions.yaml | 7 ++-- custom-openapi/feedback/openapi/feedback.yaml | 12 +++++-- .../isochrone/openapi/isochrone.yaml | 3 +- .../map-match/openapi/map-match.yaml | 10 +++--- custom-openapi/matrix/openapi/matrix.yaml | 34 ++++++++----------- custom-openapi/places/openapi/places.yaml | 13 ++++--- src/account_usage.rs | 8 ++--- src/agent_skills.rs | 2 +- src/commands.rs | 2 +- src/config.rs | 6 ++++ src/help_layout.rs | 5 +-- src/main.rs | 8 ----- src/mcp.rs | 9 ++--- src/skill_dest.rs | 8 ++--- src/uninstall.rs | 7 ++-- 15 files changed, 69 insertions(+), 65 deletions(-) diff --git a/custom-openapi/directions/openapi/directions.yaml b/custom-openapi/directions/openapi/directions.yaml index 6265012..7392c26 100644 --- a/custom-openapi/directions/openapi/directions.yaml +++ b/custom-openapi/directions/openapi/directions.yaml @@ -58,8 +58,7 @@ paths: live traffic), `mapbox/driving`, `mapbox/walking`, or `mapbox/cycling` are documented, but not necessarily exhaustive: some accounts have additional profiles of their - own. Sent exactly as typed; the API is the authority on - whether a value is valid, not this description. + own. Any other value is sent as typed. schema: type: string minLength: 1 @@ -251,8 +250,8 @@ paths: required: false description: >- Zero-based indices into `coordinates` marking which ones get - their own arrival instruction — must include `0` and the last - index. Requires `steps=true`. + their own arrival instruction, semicolon-separated — must + include `0` and the last index. Requires `steps=true`. schema: type: string example: "0,2" diff --git a/custom-openapi/feedback/openapi/feedback.yaml b/custom-openapi/feedback/openapi/feedback.yaml index 42af12b..e6fe9d1 100644 --- a/custom-openapi/feedback/openapi/feedback.yaml +++ b/custom-openapi/feedback/openapi/feedback.yaml @@ -26,8 +26,8 @@ paths: Every feedback item on the account, oldest received first by default — filterable by id, status, category, a free-text search phrase, trace id, or a time window on when it was created, - received, or last updated. Paginated with `--after`/`start_cursor`/ - `end_cursor` the way every other listing on this CLI is. + received, or last updated. Paginated: pass a response's + `end_cursor` to `--after` for the next page. parameters: - name: "access_token" in: query @@ -203,11 +203,19 @@ paths: description: >- The feedback category. Not checked against a list; `feedback list` shows the ones already in use. + # The API accepts any coordinates here and never returns the + # feature, so the order is GeoJSON's by definition rather than + # by test: on 2026-10-23 both [95, 24.94] and [24.94, 95] were + # accepted. docs.mapbox.com says [latitude, longitude]. feature: type: object description: >- A GeoJSON Feature the feedback is about, with coordinates in GeoJSON's [longitude, latitude] order. + # Verified against production on 2026-10-23: bare base64 is a + # 400 ("Image must be a base64 data URI of one of: image/png, + # image/jpeg, image/jpg"), a data URI is accepted. + # docs.mapbox.com says plain base64, which is wrong. screenshot: type: string description: >- diff --git a/custom-openapi/isochrone/openapi/isochrone.yaml b/custom-openapi/isochrone/openapi/isochrone.yaml index 4296f9a..dfc8a4e 100644 --- a/custom-openapi/isochrone/openapi/isochrone.yaml +++ b/custom-openapi/isochrone/openapi/isochrone.yaml @@ -47,8 +47,7 @@ paths: live traffic), `mapbox/driving`, `mapbox/walking`, or `mapbox/cycling` are documented, but not necessarily exhaustive: some accounts have additional profiles of their - own. Sent exactly as typed; the API is the authority on - whether a value is valid, not this description. + own. Any other value is sent as typed. schema: type: string minLength: 1 diff --git a/custom-openapi/map-match/openapi/map-match.yaml b/custom-openapi/map-match/openapi/map-match.yaml index 0e0ad0a..058434d 100644 --- a/custom-openapi/map-match/openapi/map-match.yaml +++ b/custom-openapi/map-match/openapi/map-match.yaml @@ -49,8 +49,7 @@ paths: live traffic), `mapbox/driving`, `mapbox/walking`, or `mapbox/cycling` are documented, but not necessarily exhaustive: some accounts have additional profiles of their - own. Sent exactly as typed; the API is the authority on - whether a value is valid, not this description. + own. Any other value is sent as typed. schema: type: string minLength: 1 @@ -95,9 +94,10 @@ paths: in: query required: false description: >- - Which side of the road to approach each waypoint from, - semicolon-separated — `unrestricted` or `curb` per coordinate. - Requires `--steps`. + Which side of the road to approach each waypoint from, one per + waypoint (every coordinate unless `--waypoints` picks some), + semicolon-separated — `unrestricted` or `curb`. Requires + `--steps`. schema: type: string - name: "geometries" diff --git a/custom-openapi/matrix/openapi/matrix.yaml b/custom-openapi/matrix/openapi/matrix.yaml index f7b9edf..7304172 100644 --- a/custom-openapi/matrix/openapi/matrix.yaml +++ b/custom-openapi/matrix/openapi/matrix.yaml @@ -46,13 +46,12 @@ paths: # named beyond `driving`/`walking`/`cycling`. Same fix as # `directions.yaml`'s `profile`. description: >- - The routing profile. `mapbox/driving-traffic` (accounts for - live traffic, caps at 10 coordinates instead of 25), - `mapbox/driving`, `mapbox/walking`, and `mapbox/cycling` are + The routing profile — `mapbox/driving-traffic` (accounts for + live traffic, 10 coordinates at most instead of 25), + `mapbox/driving`, `mapbox/walking`, or `mapbox/cycling` are documented, but not necessarily exhaustive: some accounts have - additional profiles of their own. Sent exactly as typed, the - API is the authority on whether a value is valid, not this - description. + additional profiles of their own. Any other value is sent as + typed. schema: type: string minLength: 1 @@ -100,17 +99,18 @@ paths: filtering the road segments considered by direction of travel. schema: type: string + # Semicolons, never commas: verified against production, a + # comma-separated list is a 422 ("may be \"all\" or + # semicolon-separated list of 0-based integer indices"). Other index + # lists on the Navigation commands vary per parameter, so this one + # says so explicitly. - name: "sources" in: query required: false description: >- - Which coordinates act as sources (matrix rows) — `all` - (the default), or zero-based indices, semicolon-separated. - Verified against production: a comma-separated list is a 422, - "may be \"all\" or semicolon-separated list of 0-based integer - indices" — unlike every other index/value list on this CLI's - Navigation commands, which are comma- or semicolon-separated - per parameter but never comma where this API wants semicolons. + Which coordinates act as sources (matrix rows) — `all` (the + default), or zero-based indices separated by semicolons + (`0;2`), not commas. schema: type: string example: "0;2" @@ -119,18 +119,14 @@ paths: required: false description: >- Which coordinates act as destinations (matrix columns) — `all` - (the default), or zero-based indices, semicolon-separated. See - `sources` above — comma-separated is a 422 here specifically. + (the default), or zero-based indices separated by semicolons + (`0;2`), not commas. schema: type: string example: "1;3" - name: "fallback_speed" in: query required: false - # Content before "Legacy", not after: `first_sentence` in - # `src/main.rs` cuts a `--help` line at the first `.`, and - # "Legacy." on its own left `--help` showing just that word. - # `--schema` and `docs/commands.md` still show it in full. description: >- Replaces a `null` (unreachable) cell with a straight-line estimate at this speed, km/h, rather than leaving it `null`. diff --git a/custom-openapi/places/openapi/places.yaml b/custom-openapi/places/openapi/places.yaml index 52f00e3..b89c151 100644 --- a/custom-openapi/places/openapi/places.yaml +++ b/custom-openapi/places/openapi/places.yaml @@ -36,12 +36,15 @@ paths: post: operationId: batch summary: Full detail for one or more places. + # `places` has no subcommand, so this, not `info.description`, is + # what `mapbox places --help` shows. It always sends the batch + # request, one id or many, so there is no get/batch choice to offer. description: >- - Each `mapbox_id` comes from a Search Box API result (`search - forward`/`reverse`/`category`) — this API does not look places up - by name or location itself. Always sends a batch request, one id - or many, so a single id reads the same way, no `get`/`batch` - choice to make. Up to 100 ids in one call. + Full detail for one or more places — hours, phone, website, + photos, address, coordinates — by the `mapbox_id`s a Search Box + result returned (`search forward`, `reverse` or `category`). Up to + 100 ids per call. Public Preview, with a 1000-records-per-account + monthly quota. parameters: - name: "access_token" in: query diff --git a/src/account_usage.rs b/src/account_usage.rs index f5236af..23e62e5 100644 --- a/src/account_usage.rs +++ b/src/account_usage.rs @@ -20,7 +20,6 @@ use crate::spec::{parse_spec, Operation}; // `--help` points at the tracker rather than at one issue in it: issue // numbers do not survive a repository move, and a dead number in help text // printed by every shipped binary is worse than no number at all. -use crate::REPO_URL; pub const COMMAND: &str = "usage"; @@ -74,13 +73,12 @@ fn operation() -> &'static Operation { pub fn command() -> Command { Command::new(COMMAND) .about("Show account and token usage by product and day") - .long_about(format!( + .long_about( "Show usage per Mapbox product, by day, for the account or one token.\n\n\ Calls the Statistics API; the token needs the `statistics:read` scope. \ `mapbox auth login` requests it by default; log in again if your stored \ - token predates that.\n\n\ - See {REPO_URL}/issues." - )) + token predates that.", + ) .arg( Arg::new(TOKEN_ID_ARG) .long(TOKEN_ID_ARG) diff --git a/src/agent_skills.rs b/src/agent_skills.rs index 83d2340..96bcabb 100644 --- a/src/agent_skills.rs +++ b/src/agent_skills.rs @@ -203,7 +203,7 @@ fn ref_arg() -> Arg { .long(REF_ARG) .value_name("REF") .default_value(DEFAULT_REF) - .help("Branch, tag or commit to install from. A commit SHA pins the install") + .help("Branch, tag or commit of the skills repository. A commit SHA pins it") } /// The archive URL for one ref. diff --git a/src/commands.rs b/src/commands.rs index def572a..11cf6c7 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -646,7 +646,7 @@ mod tests { ( "docs.tsv", include_str!("../tests/fixtures/commands_search/docs.tsv"), - 22, + 23, 30, ), ]; diff --git a/src/config.rs b/src/config.rs index dfd9c97..694b2aa 100644 --- a/src/config.rs +++ b/src/config.rs @@ -179,6 +179,12 @@ pub fn command() -> Command { Arg::new("key") .required(true) .value_parser(PossibleValuesParser::new(KEYS)) + // Defaults as `resolve` reads them. + .help( + "update-check: look for a newer version (default on); history: record \ + each run (on); log: keep diagnostic detail for recorded runs, needs \ + history (off); telemetry: send usage events to Mapbox (on)", + ) }; Command::new(COMMAND) diff --git a/src/help_layout.rs b/src/help_layout.rs index dc035c2..020e203 100644 --- a/src/help_layout.rs +++ b/src/help_layout.rs @@ -6,8 +6,9 @@ //! when they serve the same task: //! //! - **Maps and data**: create or fetch map content — styles, sprites, -//! fonts, tiles, static images. -//! - **Search**: find places and addresses, and report problems with them. +//! fonts, tiles, static images — and read or submit reports of wrong map +//! data. +//! - **Search**: find places and addresses. //! - **Navigation**: routes, travel times and reachable areas, and snapping //! GPS traces to roads. //! - **Account**: credentials, tokens and usage — who you are and what you diff --git a/src/main.rs b/src/main.rs index a2a12e7..fb0bb13 100644 --- a/src/main.rs +++ b/src/main.rs @@ -62,14 +62,6 @@ use output::{CliError, Mode}; use remedy::Remedy; use spec::ServiceSpec; -/// Where this CLI's source and issue tracker live, written down once so that -/// a repository rename is one edit rather than a grep. -/// -/// The two installers keep a copy of their own each. They are downloaded and -/// run on their own, with nothing of this crate beside them, so this is the -/// single source for the Rust half rather than for all three. -const REPO_URL: &str = repo_url!(); - /// Opens the top-level help when an agent is driving the CLI. Agents explore /// by chaining `--help` calls, one command at a time; `--schema` answers the /// same question for every command in one document. diff --git a/src/mcp.rs b/src/mcp.rs index 5b22d92..c49638c 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -352,17 +352,18 @@ pub fn command() -> ClapCommand { belongs to the agent and may already list other servers, so this shells out to \ the agent's own tooling (`claude mcp add`, `codex mcp add`, `code --add-mcp`) \ rather than editing that store directly.\n\n\ - Against the hosted Mapbox MCP endpoints only — no token, no npm package, no Node \ - version to manage. `mapbox mcp list` names every server and client this command \ + Uses the hosted Mapbox MCP endpoints: no Mapbox token to paste, no npm package, \ + no Node version to manage. The server asks the agent to sign in on first use. \ + `mapbox mcp list` names every server and client this command \ knows about.", ) .subcommand_required(true) .subcommand( - ClapCommand::new(LIST).about("List known MCP servers and clients, and what's already installed"), + ClapCommand::new(LIST).about("List known MCP servers and clients, and which are already registered"), ) .subcommand( ClapCommand::new(INSTALL) - .about("Register a server with a client. With neither flag, every known server for every detected client") + .about("Register MCP servers with coding agents; with no flags, every known server with every detected agent") .long_about(format!( "Register the {} servers with a coding agent: {}. With neither flag, registers \ every known server with every detected client.", diff --git a/src/skill_dest.rs b/src/skill_dest.rs index 4da2f4b..5ceba86 100644 --- a/src/skill_dest.rs +++ b/src/skill_dest.rs @@ -537,19 +537,19 @@ pub fn args() -> Vec { .value_parser(PossibleValuesParser::new(flags)) .conflicts_with(DIR_ARG) .help( - "Agent to write skills for, repeatable. Defaults to whichever \ - agents are installed", + "Agent whose skills to use, repeatable. Defaults to every \ + agent whose home directory is present", ), Arg::new(GLOBAL_ARG) .long(GLOBAL_ARG) .action(ArgAction::SetTrue) .conflicts_with(DIR_ARG) - .help("Write to the agent's home directory, for every project, rather than this one"), + .help("Use the agent's home directory, for every project, rather than this one"), Arg::new(DIR_ARG) .long(DIR_ARG) .value_name("DIR") .value_parser(clap::value_parser!(PathBuf)) - .help("Write to this directory instead, and nowhere else"), + .help("Use this directory instead, and nowhere else"), ] } diff --git a/src/uninstall.rs b/src/uninstall.rs index be90736..bfa495d 100644 --- a/src/uninstall.rs +++ b/src/uninstall.rs @@ -34,9 +34,10 @@ pub fn command() -> Command { .about("Uninstall this CLI by removing the installed `mapbox` binary") .long_about( "Remove the `mapbox` binary this process is running from.\n\n\ - Deletes only the executable. Stored credentials, `auth logout`'s file, \ - and the separately-installed `tilesets` binary are left alone — run \ - `mapbox auth logout` first if you also want the credential store gone.", + Deletes only the executable. The stored credentials, settings, run history, \ + logs and installed workflows in `~/.mapbox` stay, as does the \ + separately-installed `tilesets` binary — run `mapbox auth logout` first to \ + remove the stored credentials.", ) .arg(executor::dry_run_arg(DRY_RUN_HELP)) } From d4154f645de108616fb11cd1a214c289fca257b4 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 23 Oct 2026 19:44:09 +0300 Subject: [PATCH 6/6] Say a workflow step's false leaves a flag out, and that snapping's schema shape changed A step's false still omits a generated boolean flag, so a step cannot turn off one that defaults to true. The README now says so, and the runner's comment gives the real reason rather than one that would apply to --flag=false on the command line too. --- CHANGELOG.md | 4 +++- src/workflow/runner.rs | 8 +++++--- workflow/README.md | 2 ++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4badb95..0e6f3e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,9 @@ that may never merge. They are not releases and are not listed here. `--snapping-include-static-closures` take a value: `true` or `false` per coordinate, semicolon-separated (`true;false`). They were switches that sent a single `true`, which the API rejects with a 422 for any route, so - no working command changes. + no working command changes. In `--schema` they change from a boolean + flag to a string option; this is a fix rather than a breaking change for + the same reason. - `--help` shows each parameter's whole description rather than its first sentence, so the allowed values and what else a parameter needs ("Requires `overview=full`") are no longer cut off. diff --git a/src/workflow/runner.rs b/src/workflow/runner.rs index b566afa..437d644 100644 --- a/src/workflow/runner.rs +++ b/src/workflow/runner.rs @@ -679,9 +679,11 @@ fn find_arg<'a>( .ok_or_else(|| format!("`{key}` is not an argument of this command"))?; let repeatable = matches!(arg.get_action(), ArgAction::Append); // A generated boolean flag also takes `=false`, but a step's `false` - // still leaves it out: an API that reads presence rather than value - // would take `x=false` as on, and leaving it out is what `false` has - // always meant here. + // still leaves it out, so existing workflows send the same requests. + // Sending `x=false` instead would add a parameter to every request that + // wrote `false`, and nothing has checked each API accepts that. The cost + // is that a step cannot turn off a flag that defaults to true; the + // README says so. let takes_false = arg.is_require_equals_set() && arg.get_num_args() == Some((0..=1).into()); let shape = if takes_false || matches!(arg.get_action(), ArgAction::SetTrue | ArgAction::SetFalse) { diff --git a/workflow/README.md b/workflow/README.md index f5a07e4..f21a6c9 100644 --- a/workflow/README.md +++ b/workflow/README.md @@ -78,6 +78,8 @@ result: | # optional; what text mode prints instead of t A step is a `command` or a `script`, never both. +A flag set to `false` in a step's `args` is left out of the command, the same as not naming it. So a step cannot turn off a flag that defaults to true, such as `static get-image`'s `attribution`; a script step can, by running `$MAPBOX_CLI static get-image ... --attribution=false`. + ### Versions A workflow's `version` is its own, not the CLI's: workflows are released on their own schedule. It is optional, and a workflow without one installs and runs the same. `list`, `show` and `install` report it. A release is tagged `workflow--`, such as `workflow-copy-style-0.1.0`, and installed with `--ref` set to that tag.