diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6030012..6b924d1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,8 @@ that may never merge. They are not releases and are not listed here.
## Unreleased
+- Clean up.
+
### Added
- The README now documents installing without the install script: the
diff --git a/Cargo.lock b/Cargo.lock
index ce69239..c2f138e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -751,7 +751,7 @@ checksum = "4050469837a6ff301cd14c1f8f24f88549e6d548f24f64e2148eb0f72cebc51f"
[[package]]
name = "mapbox-cli"
-version = "0.2.2"
+version = "0.3.0"
dependencies = [
"anyhow",
"base64",
diff --git a/Cargo.toml b/Cargo.toml
index 8cb4407..6a51d0b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
[package]
name = "mapbox-cli"
-version = "0.2.2"
+version = "0.3.0"
edition = "2021"
description = "A command-line interface for Mapbox APIs, with commands generated at build time from OpenAPI specs."
repository = "https://github.com/mapbox/cli"
diff --git a/docs/commands.md b/docs/commands.md
index 525ed1b..03758e3 100644
--- a/docs/commands.md
+++ b/docs/commands.md
@@ -1,6 +1,6 @@
# Implemented commands
-Every command the CLI ships: four auth commands, 34 API operations across 11
+Every command the CLI ships: four auth commands, 33 API operations across 10
command groups, the tilesets-cli proxy, `completion` and `generate-skills`. Each is
shown in both of its renderings. Which one you get is decided by `--output`, whose default
(`auto`) reads stdout: a terminal gets the left column, a pipe or redirect
@@ -10,7 +10,7 @@ gets the right one. See
Account names, style ids and tokens in the examples are replaced; everything
else is as the API sent it.
-**29 of the 34 were run against the live API and show what came back:** 26
+**29 of the 33 were run against the live API and show what came back:** 26
on 2026-09-01, and `fonts list`, `fonts upload` and `fonts delete` on
2026-09-08, once `fonts:list`/`fonts:write` became
registrable. The write operations were exercised as round trips on
@@ -29,12 +29,10 @@ commands and the flags they take — is held to `mapbox --schema` on every
`cargo test` run by `tests/docs_contract.rs`, so the half of this page that
can be checked cannot fall behind the binary.
-The remaining 5 give the response shape from the spec or the docs instead
-of a live capture, for two different reasons. `rasterarrays get-mrt-tile`
-needs a raster-array job this account does not have. The other four are
-`search`'s: read-only and safe to run, but the credentials used to write
-this page have no Search Box API access, so every call answers 401 rather
-than a result.
+The remaining 4 give the response shape from the spec or the docs instead
+of a live capture: they're all `search`'s — read-only and safe to run, but
+the credentials used to write this page have no Search Box API access, so
+every call answers 401 rather than a result.
Each **Parameters** section lists only what is specific to its command. The
globals every API command takes are
@@ -102,7 +100,6 @@ nests, and is typed `mapbox styles draft get`.
**[Tilesets](#tilesets)** —
[tilesets.get-tile](#mapbox-tilesets-get-tile) ·
[tilesets.get-mvt](#mapbox-tilesets-get-mvt) ·
-[tilesets.get-mrt](#mapbox-tilesets-get-mrt) ·
[tilesets.query](#mapbox-tilesets-query)
**[Tilesets CLI](#tilesets-cli)** — [tilesets-cli](#mapbox-tilesets-cli-args)
@@ -349,7 +346,7 @@ Docs: https://docs.mapbox.com/api/accounts/tokens/
## API command groups
-34 operations across 11 command groups. Ten are generated from the OpenAPI specs
+33 operations across 10 command groups. Nine are generated from the OpenAPI specs
vendored in `openapi/`; `search` is the one exception — a hand-authored
spec versioned in this repo's own `custom-openapi/`, see
`custom-openapi/README.md`.
@@ -367,7 +364,6 @@ operation moved to `tilesets`.
| [`accounts`](#accounts) | **3** |
| [`fonts`](#fonts) | **3** |
| [`geocoder`](#geocoder) | **3** |
-| [`rasterarrays`](#raster-arrays) | **1** |
| [`search`](#search) | **4** |
| [`sprites`](#sprites) | **5** |
| [`static-images`](#static-images) | **3** |
@@ -376,7 +372,7 @@ operation moved to `tilesets`.
| [`tilequery`](#tilequery) | **1** |
| [`tilesets`](#tilesets) | **2** |
-The [Contents](#contents) list above names every one of the 34.
+The [Contents](#contents) list above names every one of the 33.
**Everything else the Mapbox specs describe is not here at all.** Not
hidden, not shipped as a command that refuses: absent from the spec content
@@ -487,7 +483,7 @@ Three things worth knowing about the read forms:
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` — 12 of
-the 34 operations — plus `auth login`, `auth logout`, `auth refresh` and
+the 33 operations — plus `auth login`, `auth logout`, `auth refresh` and
`generate-skills`. A read-only `GET` does not take it, so `mapbox styles
list --dry-run` is a usage error rather than a no-op. It rehearses
rather than describes: `--data` is parsed and every `--file` is read, so a
@@ -568,8 +564,8 @@ properties directly. A conforming response never reaches that case:
`geocoder` requires `name`/`feature_type` on every feature, `tilequery`
requires `tilequery.layer`.
-Four of the eleven command groups can answer with bytes — `rasterarrays`,
-`static-images`, `static-tiles` and `tilesets`. Those bypass `--output` in
+Three of the ten command groups can answer with bytes — `static-images`,
+`static-tiles` and `tilesets`. Those bypass `--output` in
both modes:
@@ -817,14 +813,8 @@ afterward.
### `mapbox fonts list`
-The font faces an account owns.
-
-#### Parameters
-
-`--fresh` skips the cache. Worth knowing even outside a script: a font
-just uploaded does not appear without it — the API's own `Cache-Control`
-otherwise serves a listing from before the upload for some window
-afterward.
+The font faces an account owns. Cached: a font just uploaded may not appear
+for some window afterward, per the API's own `Cache-Control`.
#### Examples
@@ -1915,8 +1905,7 @@ mapbox styles get ckstyle00000000000000001a --username user -o json > style.json
— is served through CloudFront with `max-age=900`, so a `styles get` right
after an `styles update` may still show the old document. The mutation
commands return the new state themselves, from the origin; trust that rather
-than reading back. `list-files --fresh true` is the one read that rebuilds
-instead of being served from cache.
+than reading back.
A style document is a nested tree of layers and sources, not rows, so it
stays JSON in both modes — indented under `text`, one line under `json`.
@@ -2247,18 +2236,17 @@ beforehand.
## Tilesets
-Tiles by tileset id, plus the raster-array and vector-tile lookups that key
-off one: raster from the Raster Tiles API, vector from the Vector Tiles
-API, MRT tiles from a raster-array job, and `query` from the Tilequery
-API. Four operations, out of four different specs, under one command
-group (#116) — a caller asking about a tileset is asking the same kind of
-question regardless of which API answers it.
+Tiles by tileset id, plus the vector-tile lookup that keys off one: raster
+from the Raster Tiles API, vector from the Vector Tiles API, and `query`
+from the Tilequery API. Three operations, out of three different specs,
+under one command group (#116) — a caller asking about a tileset is asking
+the same kind of question regardless of which API answers it.
Not to be confused with [`mapbox tilesets-cli`](#tilesets-cli), which
forwards to the separately installed Python Tilesets CLI and shares
nothing with this but the word.
-The three tile commands return bytes, so `--output` does not apply on
+The two tile commands return bytes, so `--output` does not apply on
them — redirect to a file. `query` returns GeoJSON.
### `mapbox tilesets get-tile`
@@ -2355,44 +2343,6 @@ $ ls -l tile.mvt
-### `mapbox tilesets get-mrt`
-
-One MRT tile from a raster-array job.
-
-#### Parameters
-
-`--jobid ` is required and comes from the raster-array job that
-produced the tiles.
-
-#### Examples
-
-```sh
-mapbox tilesets get-mrt --jobid 12 2048 1361 > tile.mrt
-```
-
-#### Outputs
-
-Not exercised: this account has no raster-array job. A `--jobid` that does
-not exist gets a 500 rather than a 404, so the error says nothing useful
-about what was wrong:
-
-
-
-```json
-{"code":"http_500","message":"Internal Server Error","status":500}
-```
-
-
-
-
### `mapbox tilesets query`
What features a vector tileset has at or near a point.
diff --git a/openapi/api-accounts/tokens-api.yaml b/openapi/api-accounts/tokens-api.yaml
index 9191eee..aba0233 100644
--- a/openapi/api-accounts/tokens-api.yaml
+++ b/openapi/api-accounts/tokens-api.yaml
@@ -217,14 +217,6 @@ components:
description: The username of the account that owns the tokens.
schema:
type: string
- TokenId:
- name: token_id
- in: path
- required: true
- description: The unique identifier of the token (the `id` field of a token object), not the token
- string itself.
- schema:
- type: string
responses:
BadRequest:
description: The request was malformed — for example, the token string was supplied where a token
diff --git a/openapi/api-fonts/fonts.production.v1.yaml b/openapi/api-fonts/fonts.production.v1.yaml
index c970fe1..0e53fb1 100644
--- a/openapi/api-fonts/fonts.production.v1.yaml
+++ b/openapi/api-fonts/fonts.production.v1.yaml
@@ -18,28 +18,14 @@ servers:
security:
- AccessTokenQuery: []
tags:
-- name: health
- description: API health check
- name: fonts
description: Manage and retrieve custom fonts
-- name: fontstacks
- description: Retrieve Mapbox fontstack glyphs
-- name: models
- description: Retrieve 3D model assets
components:
securitySchemes:
AccessTokenQuery:
type: apiKey
in: query
name: access_token
- parameters:
- fresh:
- name: fresh
- in: query
- required: false
- schema:
- type: boolean
- description: If true, sets Cache-Control to no-cache.
schemas:
Error:
type: object
@@ -113,7 +99,6 @@ paths:
security:
- AccessTokenQuery: []
parameters:
- - $ref: '#/components/parameters/fresh'
- name: owner
in: path
required: true
diff --git a/openapi/api-gl/static-images.production.v1.yaml b/openapi/api-gl/static-images.production.v1.yaml
index 8859e6d..dc402ef 100644
--- a/openapi/api-gl/static-images.production.v1.yaml
+++ b/openapi/api-gl/static-images.production.v1.yaml
@@ -158,22 +158,6 @@ components:
schema:
type: string
example: pin-s+ff0000(-118.2437,34.0522),pin-s+0000ff(-118.2537,34.0422)
- bbox:
- name: bbox
- in: path
- required: true
- description: |
- Bounding box as `[minlon,minlat,maxlon,maxlat]`. Coordinates are
- WGS84 longitude/latitude. `minlon` must be less than `maxlon` and
- `minlat` less than `maxlat`, otherwise the request returns `422`.
-
- The square brackets may be sent literally or percent-encoded as `%5B`
- and `%5D`; both are accepted. Note that `curl` treats `[` and `]` as
- glob characters, so a literal bounding box on the command line needs
- either the percent-encoded form or the `--globoff` flag.
- schema:
- type: string
- example: '[-77.043686,38.892035,-76.933086,38.942035]'
lon:
name: lon
in: path
@@ -311,24 +295,6 @@ components:
schema:
type: string
example: waterway-label
- padding:
- name: padding
- in: query
- required: false
- description: |
- Padding in pixels around the overlay or bounding box. Accepts 1 to 4
- comma-separated integers (CSS shorthand order: top, right, bottom,
- left). Only applies to `auto` and bounding box positioning; using it on
- any other endpoint returns `422`.
-
- Left plus right padding must be less than `width`, and top plus bottom
- padding must be less than `height`, otherwise the request returns
- `422`.
-
- Defaults to 5% of the smallest dimension, up to a maximum of 12 pixels.
- schema:
- type: string
- example: 50,20,50,20
addlayer:
name: addlayer
in: query
diff --git a/openapi/api-rasterarrays/rasterarrays.production.v1.yaml b/openapi/api-rasterarrays/rasterarrays.production.v1.yaml
deleted file mode 100644
index 9f8488d..0000000
--- a/openapi/api-rasterarrays/rasterarrays.production.v1.yaml
+++ /dev/null
@@ -1,163 +0,0 @@
-openapi: 3.1.0
-info:
- version: 1.0.0
- title: Rasterarray Tiles API
- description: Serves MRTs
- x-mapbox-docs-path: maps/raster-arrays
- x-mapbox-docs-products:
- - Rasterarray Tiles API
- x-mapbox-docs-description: |
- The Rasterarrays API serves multi-band raster tiles in MRT (Mapbox Raster Tile) format.
-tags:
-- name: Health
- description: Service health and uptime probes
-servers:
-- url: https://api.mapbox.com/rasterarrays/v1
- description: Production
-security:
-- AccessTokenQuery: []
-paths:
- /{tilesets-id}/{z}/{x}/{y}.mrt:
- get:
- summary: Get MRT tile
- description: "Retrieves a multi-band raster tile in MRT (Mapbox Raster Tile) format.\n\nThe tileset\
- \ ID follows the format `username.id`. The jobid parameter can be found \nin the mapdoc via the\
- \ api-tileset endpoint: \n`https://api.mapbox.com/v4/.json?secure&access_token=`\n"
- parameters:
- - name: tilesets-id
- in: path
- required: true
- description: The map id (tileset) to get tiles from - username.id format
- schema:
- type: string
- minLength: 1
- example: username.tileset_id
- - name: z
- in: path
- required: true
- description: The z value of the tile request
- schema:
- type: integer
- minimum: 0
- example: 14
- - name: x
- in: path
- required: true
- description: The x value of the tile request
- schema:
- type: integer
- minimum: 0
- example: 2535
- - name: y
- in: path
- required: true
- description: The y value of the tile request
- schema:
- type: integer
- minimum: 0
- example: 3142
- - name: access_token
- in: query
- required: true
- description: Required Mapbox API Access Token
- schema:
- type: string
- minLength: 1
- example: YOUR_MAPBOX_ACCESS_TOKEN
- - name: jobid
- in: query
- required: true
- description: Required job identifier used to locate the tile. Can be found in mapdoc via api-tileset
- endpoint.
- schema:
- type: string
- minLength: 1
- example: clnf3oau5005g08kzhr460vut
- responses:
- '200':
- description: Successfully retrieved MRT tile data
- content:
- application/octet-stream:
- schema:
- type: string
- format: binary
- description: Multi-band raster tile data in MRT format
- examples:
- getMrtTile:
- summary: MRT tile binary data
- externalValue: https://api.mapbox.com/rasterarrays/v1/username.tileset_id/14/2535/3142.mrt?access_token=YOUR_MAPBOX_ACCESS_TOKEN&jobid=clnf3oau5005g08kzhr460vut
- '400':
- description: Bad Request
- content:
- application/json:
- schema:
- $ref: '#/components/schemas/Error'
- text/plain:
- schema:
- type: string
- example: Bad Request
- '403':
- description: Forbidden
- content:
- application/json:
- schema:
- $ref: '#/components/schemas/Error'
- text/plain:
- schema:
- type: string
- example: Forbidden
- '404':
- description: Not Found
- content:
- application/json:
- schema:
- $ref: '#/components/schemas/Error'
- text/plain:
- schema:
- type: string
- example: Not Found
- x-mapbox-docs-examples:
- - example-id: getMrtTile
- title: Get an MRT tile
- request:
- tilesets-id: username.tileset_id
- z: 14
- x: 2535
- y: 3142
- access_token: YOUR_MAPBOX_ACCESS_TOKEN
- jobid: clnf3oau5005g08kzhr460vut
- operationId: getMrtTile
- x-mapbox-cli-command:
- - tilesets
- - get-mrt
-components:
- securitySchemes:
- AccessTokenQuery:
- type: apiKey
- in: query
- name: access_token
- description: Mapbox access token
- schemas:
- Error:
- type: object
- properties:
- message:
- type: string
- status:
- type: integer
- errors:
- type: array
- items:
- type: string
- code:
- type: string
- required:
- - message
- additionalProperties: true
- responses:
- ErrorResponse:
- description: Error response
- content:
- application/json:
- schema:
- $ref: '#/components/schemas/Error'
diff --git a/openapi/api-rastertiles/rastertiles.production.v1.yaml b/openapi/api-rastertiles/rastertiles.production.v1.yaml
index 2c5daa8..3d91e3b 100644
--- a/openapi/api-rastertiles/rastertiles.production.v1.yaml
+++ b/openapi/api-rastertiles/rastertiles.production.v1.yaml
@@ -19,14 +19,8 @@ servers:
- url: https://api.mapbox.com
description: Production server
tags:
-- name: health
- description: Service-level health and uptime probes
- name: tiles
description: Raster tile delivery endpoints (v4)
-- name: grid
- description: UTFGrid JSON endpoints (v4 and legacy)
-- name: legacy
- description: Deprecated or legacy endpoints (API versions 1–3)
security:
- ApiKeyAuth: []
paths:
diff --git a/openapi/api-styles/styles.production.v1.yaml b/openapi/api-styles/styles.production.v1.yaml
index 2ff5179..7b534c4 100644
--- a/openapi/api-styles/styles.production.v1.yaml
+++ b/openapi/api-styles/styles.production.v1.yaml
@@ -34,10 +34,6 @@ tags:
description: Create, read, update, and delete map styles
- name: Sprites
description: Manage SVG images within a style's sprite
-- name: Folders
- description: Organize styles, tilesets, and datasets into folders
-- name: Sessions
- description: Billing session tracking endpoints
paths:
/styles/v1/{username}:
get:
diff --git a/src/executor.rs b/src/executor.rs
index b21a0a5..cfe6921 100644
--- a/src/executor.rs
+++ b/src/executor.rs
@@ -11,9 +11,10 @@ use crate::output::{self, CliError, Mode};
use crate::remedy::{self, Remedy};
use crate::spec::{Operation, Parameter, RequestBody, ACCOUNT_PLACEHOLDERS, MULTIPART};
-/// The query parameter the access token travels in, and what stands in for it
-/// anywhere the URL is shown. Named once because getting this wrong leaks a
-/// live token: `--debug` and the dry-run plan both render the same URL.
+/// The query parameter the access token travels in, and what stands in for
+/// it anywhere the URL is shown. Named once because getting this wrong
+/// leaks a live token — `--debug` and the dry-run plan both render the
+/// same URL.
const ACCESS_TOKEN: &str = "access_token";
const REDACTED: &str = "";
@@ -21,9 +22,9 @@ const REDACTED: &str = "";
const JSON_CONTENT_TYPE: &str = "application/json";
/// Name of the flag that stops short of sending. Declared per operation
-/// rather than globally: a `GET` has nothing to preview, and a flag offered
-/// where it cannot mean anything is worse than one you have to put after the
-/// operation name. See [`dry_run_arg`].
+/// rather than globally: a `GET` has nothing to preview, and offering a
+/// flag that means nothing is worse than requiring it after the operation
+/// name. See [`dry_run_arg`].
pub const DRY_RUN_ARG: &str = "dry-run";
/// The wording for a command whose mutation is an API call.
@@ -32,11 +33,11 @@ pub const DRY_RUN_REQUEST_HELP: &str =
/// The `--dry-run` flag, with the sentence that fits what the command does.
///
-/// The help text is a parameter because the two kinds of mutating command
-/// change different things: a generated operation would send a request, while
-/// `mapbox auth logout` deletes a file and sends nothing at all. A `--help`
-/// line that describes the wrong one is the small kind of lie that stops a
-/// safety flag from being reached for.
+/// The help text is a parameter because mutating commands change different
+/// things: a generated operation sends a request, while `mapbox auth
+/// logout` deletes a local file and sends nothing at all. A `--help` line
+/// that describes the wrong one is a small lie that makes people trust a
+/// safety flag less.
pub fn dry_run_arg(help: &'static str) -> clap::Arg {
clap::Arg::new(DRY_RUN_ARG)
.long(DRY_RUN_ARG)
@@ -46,8 +47,8 @@ pub fn dry_run_arg(help: &'static str) -> clap::Arg {
/// Whether `--dry-run` was given, for an operation that may not have it.
///
-/// `get_flag` panics on an argument that was never registered, and read-only
-/// operations never register this one.
+/// `get_flag` panics on an unregistered argument, and read-only operations
+/// never register this one — hence `try_get_one`.
pub fn wants_dry_run(matches: &ArgMatches) -> bool {
matches
.try_get_one::(DRY_RUN_ARG)
@@ -58,34 +59,34 @@ pub fn wants_dry_run(matches: &ArgMatches) -> bool {
}
/// The three switches that change whether and how a request goes out, as
-/// opposed to what is being requested.
+/// opposed to what's being requested.
///
-/// Grouped once there were three of them. Clippy's argument limit is the
-/// visible reason; the better one is that a call site handing over three bare
-/// `bool`s in a row is one transposition away from a silent swap — `--debug`
-/// for `--dry-run` would send the request it promised only to describe.
-/// Naming them at the call site makes that impossible.
+/// Grouped into a struct once there were three of them. Clippy's argument
+/// limit is the visible reason; the real one is that three bare `bool`s in
+/// a row at a call site is one transposition away from a silent swap —
+/// mixing up `--debug` and `--dry-run` would send a request that was only
+/// supposed to be described. Naming the fields makes that impossible.
#[derive(Clone, Copy)]
pub struct RunFlags {
pub debug: bool,
pub assume_yes: bool,
pub dry_run: bool,
/// The budget the caller asked for, from `crate::http::requested` —
- /// `--timeout` or `MAPBOX_TIMEOUT`. `None` does not mean "no timeout": it
- /// means nobody said, and [`crate::http::budget`] falls back to whichever
- /// default fits what the request is carrying.
+ /// `--timeout` or `MAPBOX_TIMEOUT`. `None` doesn't mean "no timeout",
+ /// it means "nobody said"; [`crate::http::budget`] then falls back to
+ /// whatever default fits the request.
pub timeout: Option,
}
-/// Runs one generated command — the request it would make, sent or, under
-/// `--dry-run`, described.
+/// Runs one generated command — the request it would make, either sent or
+/// described under `--dry-run`.
///
-/// A wrapper, so that every way this can reject an *argument* is pointed at
-/// `--schema` from one place instead of three: the choice between `--data`
-/// and `--file`, a `--data` that will not parse, and the same parse again on
-/// the dry-run path, which reads the body rather than trusting it.
-/// `with_schema_action` answers to a code, so every other failure — an HTTP
-/// status, an unreadable file — passes through untouched.
+/// A thin wrapper so every way this can reject an *argument* points at
+/// `--schema` from one place, not three: choosing between `--data` and
+/// `--file`, a `--data` that won't parse, and re-parsing it on the
+/// dry-run path (which reads the body instead of trusting it).
+/// `with_schema_action` reacts to a specific error code, so everything
+/// else — an HTTP status, an unreadable file — passes through untouched.
pub fn execute(
op: &Operation,
matches: &ArgMatches,
@@ -115,14 +116,12 @@ fn dispatch(
let mut path = op.path_template.clone();
- // Substitute {username}/{owner}/{account} from global --username
if let Some(u) = username {
for placeholder in ACCOUNT_PLACEHOLDERS {
path = path.replace(&format!("{{{placeholder}}}"), u);
}
}
- // Substitute other path params from positional args
for param in &op.path_params {
if let Some(val) = matches.get_one::(¶m.arg_name) {
let safe = path_segment(¶m.name, val)?;
@@ -131,7 +130,6 @@ fn dispatch(
}
if path.contains('{') {
- // Extract missing param names for a useful error
let missing: Vec<&str> = path
.split('{')
.skip(1)
@@ -144,8 +142,8 @@ fn dispatch(
missing.join(", ")
),
)
- // The message names the two ways to supply an account; this names
- // the command that says whether a login would supply it already.
+ // The message above names the two ways to supply an account; this
+ // adds the command that checks whether a login already supplies it.
.with_remedy(Remedy::default().with_action(Some("mapbox auth whoami".to_string())))
.into());
}
@@ -168,12 +166,12 @@ fn dispatch(
}
}
- // `--data` and `--file` are declared per-operation, so either may be
- // absent from this command entirely; `get_one` panics on an argument
- // that was never registered.
- // Resolved before anything reads it, `--dry-run` included, so a `@path`
- // that does not exist fails here rather than being described as a request
- // and then failing at send time. `resolve_data` says why.
+ // `--data`/`--file` are declared per-operation, so either may not exist
+ // on this command at all — `get_one` would panic on an unregistered
+ // argument, hence `try_get_one`. Resolved here, before `--dry-run` or
+ // anything else reads it, so a `@path` that doesn't exist fails now
+ // instead of after being described as a request that would then fail
+ // to send.
let data_argument = match matches.try_get_one::("data").ok().flatten() {
Some(value) => Some(resolve_data(value)?),
None => None,
@@ -188,19 +186,20 @@ fn dispatch(
.map(|values| values.map(String::as_str).collect())
.unwrap_or_default();
- // Resolved ahead of the dry-run branch on purpose: choosing between
+ // Resolved before the dry-run branch on purpose: choosing between
// `--data` and `--file` is a check the caller can fail, and a dry run
- // that skipped it would report a request that could not actually be
- // sent — which is the one thing it exists to rule out.
+ // that skipped it would describe a request that could never actually
+ // be sent.
let body_source = match &op.body {
Some(body) => Some(resolve_body_source(body, data, &files)?),
None => None,
};
if dry_run {
- // Worth saying out loud, and only here: a real call answers a missing
- // token with a 401 the caller cannot miss, while a dry run would
- // happily print a plausible-looking request that could never succeed.
+ // Worth saying here specifically: a real call would answer a
+ // missing token with an unmissable 401, but a dry run would just
+ // print a plausible-looking request that could never actually
+ // succeed.
if token.is_none() {
output::progress(
"Note: no access token was resolved, so the real request would be unauthenticated.",
@@ -209,21 +208,17 @@ fn dispatch(
return describe_request(op, mode, &url, &query, body_source.as_ref());
}
- // Below the dry-run branch, and that ordering is the point: a dry run
- // sends nothing, so asking whether to go ahead would be asking about
- // something that is not going to happen — and it would make the flag that
- // exists to be safe the one that blocks a script.
+ // Placed after the dry-run branch deliberately: a dry run sends
+ // nothing, so asking "go ahead?" about something that isn't going to
+ // happen would make the safety flag the thing that blocks a script.
//
- // `url` is passed rather than the rendered request because it carries no
- // query string, and the access token is a query parameter. The question is
- // printed. `redacted_url` above exists for the same reason on the same
- // string; this call must never be given its output.
+ // `url` here, not the redacted one — the access token is a query
+ // parameter and `url` carries no query string at all, so it's already
+ // safe to print. Never pass `redacted_url`'s output here.
confirm::destructive_request(&op.method, &url, assume_yes)?;
- // Below the dry-run branch, so a `[debug]` request line always means a
- // request that went out. The plan renders the same URL itself, and one
- // that reads as a call having been made is the last thing this flag
- // should print.
+ // Also after the dry-run branch, so a `[debug]` line always means a
+ // request actually went out — never one that only looks like it did.
if debug {
eprintln!("[debug] {} {}", op.method, redacted_url(&url, &query));
}
@@ -238,13 +233,13 @@ fn dispatch(
_ => client.get(&url),
}
.query(&query)
- // Named on the request rather than left to the client, because the client
- // cannot know what any one request is doing: a sprite upload sends a file
- // and a listing sends nothing, and a single budget that suits both is
- // either too short for the upload or too long to be a timeout at all.
- // `reqwest` prefers the request's own over the client's — 0.12.28's
- // `execute_request` reads `req.timeout().copied().or(self.timeout.0)` —
- // so this is what actually applies to everything sent from here.
+ // Set per-request, not on the client: the client has no way to know
+ // what a given request is doing, and a sprite upload (needs a long
+ // timeout) and a listing (needs a short one) can't share one budget.
+ // `reqwest` prefers the request's own timeout over the client's —
+ // 0.12.28's `execute_request` reads
+ // `req.timeout().copied().or(self.timeout.0)` — so this is what
+ // actually applies.
.timeout(http::budget(
timeout,
payload_of(
@@ -263,32 +258,32 @@ fn dispatch(
.send()
.map_err(|e| transport_failure("Request failed", e))?;
let status = response.status();
- // Read the headers before `bytes()` consumes the response — whatever is
- // not taken here is gone by the next line. `Content-Type` was for a long
- // time the only one that survived this point, which is what made
- // pagination and support escalation unreachable; see [`ResponseHeaders`].
+ // Must read headers before `bytes()` consumes the response — anything
+ // not taken here is gone after. For a long time only `Content-Type`
+ // survived this point, which is why pagination and support escalation
+ // used to be unreachable; see [`ResponseHeaders`].
let headers = ResponseHeaders::read(response.headers());
let content_type = headers.content_type.clone();
let body = response
.bytes()
.map_err(|e| transport_failure("Failed to read response", e))?;
- // Six of the twelve services answer with bytes, not text — images, vector
- // tiles, glyph PBFs, style ZIPs. Decoding those as UTF-8 replaces every
- // invalid sequence with U+FFFD, which silently corrupts the payload: a PNG
- // arrives with its leading 0x89 rewritten to EF BF BD and no longer opens.
- // An error response is worth reading whatever the endpoint normally
- // returns, so failures always take the text path.
+ // Six of the twelve services answer with bytes, not text — images,
+ // vector tiles, glyph PBFs, style ZIPs. Decoding those as UTF-8 would
+ // silently corrupt them (a PNG's leading 0x89 becomes EF BF BD and the
+ // file no longer opens). But an error response is worth reading no
+ // matter what the endpoint normally returns, so failures always take
+ // the text path.
let as_text = if status.is_success() && is_binary_content_type(&content_type) {
None
} else {
Some(String::from_utf8_lossy(&body))
};
- // A failure is an error, not a result: it belongs on stderr in whichever
- // shape the caller asked for, so it never lands in a redirected file
- // alongside real output. The body is carried into the error rather than
- // printed here, which keeps every detail the old dump-to-stdout showed.
+ // A failure is an error, not a result, so it belongs on stderr — never
+ // in a redirected file alongside real output. The body goes into the
+ // error object instead of being printed here, keeping every detail the
+ // old approach (dumping to stdout) showed.
if !status.is_success() {
let text = as_text
.as_deref()
@@ -299,11 +294,11 @@ fn dispatch(
.into());
}
- // A 204, or a 200 that carries nothing. `serde_json` cannot parse an
- // empty string, so this fell through to the text-body path and printed a
- // blank line — or `""` under `json`, a valid document that says nothing.
- // Every mutation in the CLI that succeeds without a body landed there,
- // which left a delete indistinguishable from a command that did not run.
+ // A 204, or a 200 with nothing in it. `serde_json` can't parse an empty
+ // string, so this used to fall through to the text-body path and print
+ // a blank line (or `""` under `json` — a valid document that says
+ // nothing). Every no-body mutation ended up there, which made a
+ // successful delete look the same as a command that never ran.
if as_text
.as_deref()
.is_some_and(|text| text.trim().is_empty())
@@ -319,8 +314,7 @@ fn dispatch(
);
}
- // `Some` only when the response is one page of several, which is the
- // listings and nothing else.
+ // `Some` only for a listing that's one page of several.
let next_page = headers
.next_page
.as_deref()
@@ -335,8 +329,8 @@ fn dispatch(
.map_err(|err| with_page_context(err, next_page.as_ref()))?,
None,
Some(&op.service),
- // The row asked for is in hand; where the *other* rows
- // are is not advice about it.
+ // The wanted row is already in hand, so no need for a
+ // pagination tip about the other rows.
None,
)?,
None => output::emit_value(
@@ -349,41 +343,42 @@ fn dispatch(
},
Err(_) => output::emit_text_body(mode, &text)?,
},
- // Bytes bypass the output contract entirely. A PNG cannot be wrapped
- // in a JSON envelope without destroying it, and `--output json` on a
- // tile endpoint is far more likely to be the global flag riding along
- // than a considered request to mangle the image.
+ // Bytes bypass the output contract entirely. A PNG can't be
+ // wrapped in a JSON envelope without destroying it, and
+ // `--output json` on a tile endpoint is far more likely to be a
+ // global flag riding along than a deliberate request to mangle
+ // the image.
None => write_binary(&body, &content_type)?,
}
Ok(())
}
-/// The headers a Mapbox response may identify itself with, in the order they
-/// are preferred.
+/// The headers a Mapbox response may identify itself with, checked in this
+/// order.
///
-/// Measured rather than assumed, and the measurement is the reason there are
-/// two. `x-request-id` is the name the convention would predict and is what
-/// this looked for first — but no Mapbox endpoint reachable from here sends
-/// it: styles, tokens, fonts and geocoding v6 all answer without one, on both
-/// success and failure. What every one of them does carry is `x-amz-cf-id`,
-/// the CloudFront request id, because the whole API is fronted by it — and
-/// that is the id support traces a request with.
+/// There are two because we measured, not assumed. `x-request-id` is the
+/// conventional name and what we checked first — but no Mapbox endpoint
+/// reachable from here actually sends it: styles, tokens, fonts, and
+/// geocoding v6 all answer without one, success or failure. What they all
+/// carry instead is `x-amz-cf-id`, the CloudFront request id, since the
+/// whole API sits behind CloudFront — and that's the id support uses to
+/// trace a request.
///
-/// `x-request-id` stays first because a service that does send one means it
-/// more specifically than the CDN in front of it does, and it costs a lookup
-/// in a map that is already in memory.
+/// `x-request-id` stays first anyway: a service that does send one is more
+/// specific than the CDN in front of it, and checking costs only a lookup
+/// in a map already in memory.
const REQUEST_ID_HEADERS: [&str; 2] = ["x-request-id", "x-amz-cf-id"];
/// The request id from a response, for a caller that reads the body itself.
///
-/// `text()` and `bytes()` both consume the response, so this has to be called
-/// before the body is read — which is the whole reason it is a named function
-/// rather than a line inlined at each of the three call sites.
+/// `text()` and `bytes()` both consume the response, so this must be
+/// called before reading the body — which is why it's a named function
+/// instead of being inlined at each of the three call sites.
///
-/// Mapbox-bound requests only. `agent_skills` talks to GitHub codeload, which
-/// identifies requests with `x-github-request-id` and is not something Mapbox
-/// support can look up, so it deliberately does not call this.
+/// For Mapbox-bound requests only. `agent_skills` talks to GitHub
+/// codeload, which uses `x-github-request-id` instead — something Mapbox
+/// support can't look up — so it deliberately doesn't call this.
pub fn request_id(headers: &reqwest::header::HeaderMap) -> Option {
REQUEST_ID_HEADERS.iter().find_map(|name| {
headers
@@ -397,16 +392,15 @@ pub fn request_id(headers: &reqwest::header::HeaderMap) -> Option {
/// What a response says about itself, past its body.
///
-/// A struct rather than three reads at the call site because `bytes()`
-/// consumes the response: whatever is not taken before it is unrecoverable.
-/// Taking only `Content-Type` is what left paginated listings truncating
-/// silently and left a 500 with nothing to quote to support.
+/// A struct instead of three reads at the call site, because `bytes()`
+/// consumes the response — whatever isn't taken before that is gone for
+/// good. Taking only `Content-Type` used to leave paginated listings
+/// truncating silently, and a 500 with nothing to quote to support.
struct ResponseHeaders {
/// Decides whether the body is read as text or written as bytes.
content_type: String,
- /// The request id — what support needs to find this one request in their
- /// logs. Carried into the error and never printed on success, because on
- /// a response that worked it is noise.
+ /// What support needs to find this request in their logs. Carried
+ /// into the error and never printed on success, where it's just noise.
request_id: Option,
/// The `rel="next"` target of a `Link` header, when this response is one
/// page of several.
@@ -415,8 +409,8 @@ struct ResponseHeaders {
impl ResponseHeaders {
fn read(headers: &reqwest::header::HeaderMap) -> Self {
- // A header present but empty says nothing, and an empty request id
- // would print as `Request ID:` with a blank after it.
+ // A present-but-empty header says nothing; treating it as missing
+ // avoids printing "Request ID:" with nothing after it.
let text = |name: &str| {
headers
.get(name)
@@ -437,31 +431,29 @@ impl ResponseHeaders {
}
}
-/// A response that is one page of several, and how to ask for the next one.
+/// A response that's one page of several, and how to ask for the next one.
///
-/// Holds the flags rather than the URL: following the `Link` target verbatim
-/// would mean re-sending a URL the API built, token and all, while the flags
-/// are something a caller can read, edit and run.
+/// Holds the flags, not the URL: following the `Link` target verbatim would
+/// mean re-sending a URL the API built (token and all), while flags are
+/// something a caller can read, edit, and run.
struct NextPage(Option);
impl NextPage {
- /// Derives the flags from the operation rather than hardcoding `--start`.
+ /// Derives the flags from the operation instead of hardcoding `--start`.
///
/// The next URL's query is matched against the parameters this command
/// declares, so the tip names whatever the spec calls its paging
- /// parameters, and a service that pages some other way needs no change
- /// here.
+ /// parameters — a service that pages differently needs no change here.
///
- /// **The access token cannot appear in the result.** It rides in the
- /// query string of every request, so the API echoes it back in this very
- /// URL. Two things keep it out, and the second is why the first is not
- /// enough: `dispatch` adds it directly rather than declaring it in
- /// `op.query_params`, so matching against the declared parameters
- /// excludes it today — but a spec is free to declare a parameter by that
- /// name, and then "by construction" would quietly stop being true, so it
- /// is also refused explicitly. `the_page_tip_never_names_the_access_token`
- /// and `a_declared_parameter_named_access_token_is_still_withheld` hold
- /// both halves.
+ /// **The access token can never appear in the result.** It rides in the
+ /// query string of every request, so the API echoes it back in this
+ /// very URL. Two safeguards, not one: `dispatch` adds the token
+ /// directly rather than declaring it in `op.query_params`, so matching
+ /// against declared parameters already excludes it — but a spec could
+ /// declare a parameter with that same name someday, so it's also
+ /// refused explicitly. `the_page_tip_never_names_the_access_token` and
+ /// `a_declared_parameter_named_access_token_is_still_withheld` test
+ /// both.
fn of(declared: &[Parameter], next: &str) -> Self {
let flags: Vec = query_pairs(next)
.into_iter()
@@ -481,10 +473,10 @@ impl NextPage {
fn tip(&self) -> String {
match &self.0 {
Some(flags) => format!("More results: add `{flags}` for the next page."),
- // Reachable only if the API pages an operation whose spec
- // declares no paging parameter — a spec gap, not a user error.
- // Saying so beats saying nothing, because the result is
- // incomplete either way and only this knows it.
+ // Only reachable if the API paginates an operation whose spec
+ // declares no paging parameter — a spec gap, not a user
+ // error. Still worth saying, since the result is incomplete
+ // either way.
None => "More results exist, but this command declares no parameter to reach them."
.to_string(),
}
@@ -504,11 +496,11 @@ impl NextPage {
/// A URL's query, decoded.
///
-/// Percent-decoded on purpose: the values go into a tip meant to be copied
-/// onto a command line, and the CLI re-encodes whatever it is given — so
-/// handing back `%2B` would round-trip to `%252B` and ask for the wrong page.
-/// An unparseable URL yields nothing rather than failing: a tip is not worth
-/// turning a successful request into an error.
+/// Percent-decoded on purpose: these values go into a tip meant to be
+/// copied onto a command line, and the CLI re-encodes whatever it's given
+/// — so handing back `%2B` would round-trip to `%252B` and ask for the
+/// wrong page. An unparseable URL yields nothing rather than an error; a
+/// pagination tip isn't worth turning a successful request into a failure.
fn query_pairs(url: &str) -> Vec<(String, String)> {
match reqwest::Url::parse(url) {
Ok(parsed) => parsed
@@ -521,9 +513,9 @@ fn query_pairs(url: &str) -> Vec<(String, String)> {
/// A query value as it would have to be typed into a shell.
///
-/// Paging cursors are opaque ids in practice, but the tip is advice a reader
-/// pastes, and an unquoted value with a space in it would silently become
-/// two arguments.
+/// Paging cursors are opaque ids in practice, but this tip gets pasted
+/// verbatim, and an unquoted value with a space in it would silently
+/// become two arguments.
fn shell_value(value: &str) -> String {
let safe = |c: char| c.is_ascii_alphanumeric() || "-_.~:@+,".contains(c);
if !value.is_empty() && value.chars().all(safe) {
@@ -534,17 +526,17 @@ fn shell_value(value: &str) -> String {
/// Adds "there are more pages" to a `--id` that matched nothing.
///
-/// `pick_row` searches the rows it was handed and says "No row has the id",
-/// which is true of the page and may well be false of the listing. On a
-/// paginated response that is the most misleading form of the truncation
-/// this whole path exists to stop, so the error says which it means.
+/// `pick_row` says "No row has the id", which is true of this page but
+/// might be false of the whole listing. On a paginated response, that's
+/// the most misleading version of the truncation problem this whole path
+/// exists to prevent — so the error clarifies which one it means.
fn with_page_context(err: anyhow::Error, next_page: Option<&NextPage>) -> anyhow::Error {
let Some(next_page) = next_page else {
return err;
};
match err.downcast::() {
- // `not_a_list` is about the shape of the response, which another
- // page would not change.
+ // `not_a_list` is about the shape of the response — another page
+ // wouldn't change that.
Ok(cli) if cli.code == "not_found" => cli
.with_remedy(Remedy::default().with_fix(&next_page.fix()))
.into(),
@@ -555,10 +547,10 @@ fn with_page_context(err: anyhow::Error, next_page: Option<&NextPage>) -> anyhow
/// The request line a reader may safely see: URL, query, token replaced.
///
-/// The access token rides in the query string, so every rendering of this URL
-/// has to strip it. `--debug` has always done so; the dry run needs it more,
-/// because the request *is* its output — the thing most likely to be pasted
-/// into an issue or captured whole by CI.
+/// The access token rides in the query string, so every rendering of this
+/// URL has to strip it. `--debug` has always done this; the dry run needs
+/// it even more, since the request *is* its output — the part most likely
+/// to get pasted into an issue or captured whole by CI.
fn redacted_url(url: &str, query: &[(String, String)]) -> String {
let rendered: Vec = query
.iter()
@@ -621,9 +613,9 @@ fn encode_query_value(value: &str) -> String {
/// One query parameter's value, or the stand-in when it is the token.
///
-/// The single place that decides what may be printed. Both renderings of the
-/// query — the URL line and the JSON object — go through it, so the rule
-/// cannot come to differ between the two.
+/// The single place deciding what may be printed. Both renderings of the
+/// query — the URL line and the JSON object — go through this, so the
+/// rule can't drift between the two.
fn shown_value<'a>(name: &str, value: &'a str) -> &'a str {
if name == ACCESS_TOKEN {
REDACTED
@@ -634,11 +626,11 @@ fn shown_value<'a>(name: &str, value: &'a str) -> &'a str {
/// The dry run's answer: what the real call would send, and nothing sent.
///
-/// It leaves through `output::emit` like any other result, and it goes to
-/// stdout for the same reason — the command was asked what it would do, and
-/// this is the answer to that question, not a note about it. So `-o json`
-/// gives a script an object to assert on before it lets a delete run for
-/// real, and a terminal gets one readable request line.
+/// Goes through `output::emit` and out to stdout like any other result —
+/// the command was asked what it would do, and this *is* the answer, not
+/// a note about it. So `-o json` gives a script an object to check before
+/// letting a real delete run, and a terminal gets one readable request
+/// line.
fn describe_request(
op: &Operation,
mode: Mode,
@@ -658,9 +650,9 @@ fn describe_request(
text.push_str(&described.text);
}
- // An object, not a list of pairs: no query parameter can be repeated
- // here, since each one comes from an argument clap keeps a single value
- // for, and `.query.access_token` is worth more to a caller than
+ // An object, not a list of pairs: no query parameter repeats here,
+ // since each comes from an argument clap keeps one value for, and
+ // `.query.access_token` is more useful to a caller than
// `.query[3].value`.
let shown_query: serde_json::Map = query
.iter()
@@ -677,9 +669,9 @@ fn describe_request(
"dry_run": true,
"command": op.command(),
"method": method,
- // Without the query string, which `query` carries broken out.
- // Rejoining the two would only produce a URL nobody can use, the
- // token in it being redacted.
+ // No query string here — `query` carries that separately.
+ // Rejoining them would produce a URL nobody can use, since the
+ // token in it is redacted.
"url": url,
"query": shown_query,
"body": described.map(|described| described.json),
@@ -696,12 +688,13 @@ struct DescribedBody {
/// Describes the body — and validates it exactly as sending would.
///
-/// Every check [`attach_body`] makes is made here too: the JSON is parsed,
-/// and each `--file` is read rather than stat-ed. Reading is the point. A dry
-/// run answers "would this work?", and a file that exists but cannot be read
-/// passes a metadata check and fails the real call — which is precisely the
-/// surprise the flag exists to rule out. The operations taking `--file` take
-/// sprites and upload chunks, so the read costs nothing worth saving.
+/// Every check [`attach_body`] makes happens here too: the JSON gets
+/// parsed, and each `--file` gets read, not just stat-ed. Reading matters
+/// because a dry run answers "would this work?", and a file that exists
+/// but can't be read would pass a metadata check and then fail the real
+/// call — exactly the surprise `--dry-run` is meant to rule out. The
+/// operations that take `--file` (sprites, upload chunks) are small, so
+/// the read costs nothing worth skipping.
fn describe_body(source: &BodySource<'_>) -> Result> {
Ok(match source {
BodySource::Empty => None,
@@ -718,9 +711,9 @@ fn describe_body(source: &BodySource<'_>) -> Result> {
"source": "--data",
"content_type": JSON_CONTENT_TYPE,
"bytes": data.len(),
- // The parsed document rather than the string it was typed
- // as: re-escaping it into a JSON string would make the one
- // thing worth checking here the one thing unreadable.
+ // The parsed document, not the raw string it was typed
+ // as — re-escaping it into a JSON string would make
+ // the one thing worth checking here unreadable.
"json": json,
}),
})
@@ -758,9 +751,9 @@ fn describe_body(source: &BodySource<'_>) -> Result> {
for path in paths {
let bytes = read_body_file(path)?.len();
let media_type = part_media_type(path);
- // The part's filename, spelled out because it is not
- // cosmetic: `batchUploadSprite` takes the icon name from it,
- // so this line is what the sprite will be called.
+ // Not just cosmetic: `batchUploadSprite` takes the icon
+ // name from this filename, so it's also what the sprite
+ // gets called.
let name = file_name_of(path);
total += bytes;
text.push_str(&format!(
@@ -790,8 +783,9 @@ fn describe_body(source: &BodySource<'_>) -> Result> {
/// What to put in the request body, decided from the spec and the flags.
///
-/// Borrows rather than owns so the decision stays free of I/O: choosing is
-/// separable from reading the files, and only the choosing is worth testing.
+/// Borrows instead of owning so this decision stays free of I/O: choosing
+/// what to send is separate from reading the files, and only the choosing
+/// is worth testing on its own.
#[derive(Debug, PartialEq, Eq)]
enum BodySource<'a> {
/// The operation takes a body but the caller supplied none. Still a
@@ -820,11 +814,11 @@ enum BodySource<'a> {
struct DataArgument<'a> {
/// The body itself. Borrowed when it was typed, owned when it was read.
body: std::borrow::Cow<'a, str>,
- /// Whether it came from a file or stdin rather than from argv.
+ /// Whether it came from a file or stdin rather than argv.
///
- /// Carried because the timeout budget turns on it and on nothing else a
- /// caller can see: argv caps what can be typed at roughly a megabyte, and
- /// nothing caps a file. See [`payload_of`].
+ /// Carried because the timeout budget depends on it: argv caps what
+ /// can be typed at roughly a megabyte, but nothing caps a file. See
+ /// [`payload_of`].
streamed: bool,
}
@@ -838,18 +832,18 @@ const STDIN_PATH: &str = "-";
/// Resolves a `--data` argument that names a file instead of carrying a body.
///
/// `@path` reads the file, `@-` reads stdin, and anything else is the body
-/// itself — the spelling curl has used for long enough that it is what people
-/// try first.
+/// itself — the spelling curl has used long enough that people try it
+/// first.
///
-/// The ambiguity this inherits is curl's: a body whose first character is a
-/// literal `@` cannot be passed this way. It costs nothing here, because every
-/// operation reachable with `--data` today sends JSON, and `@` is not valid
-/// JSON. If a text body that could start with one is ever wired up, `--data-raw`
-/// is the established escape hatch.
+/// Inherits curl's ambiguity: a body whose first character is a literal
+/// `@` can't be passed this way. That costs nothing here, since every
+/// operation reachable with `--data` today sends JSON, and `@` isn't valid
+/// JSON. If a text body that could start with `@` ever gets wired up,
+/// `--data-raw` is the established escape hatch.
///
-/// Read here rather than at send time so that a `--dry-run` validates the file
-/// too. A dry run that skipped this would describe a request that could not
-/// actually be sent, which is the one thing it exists to rule out.
+/// Read here, not at send time, so `--dry-run` validates the file too — a
+/// dry run that skipped this could describe a request that would never
+/// actually send.
fn resolve_data(value: &str) -> Result> {
let Some(path) = value.strip_prefix(DATA_FROM_PATH) else {
return Ok(DataArgument {
@@ -872,11 +866,11 @@ fn resolve_data(value: &str) -> Result> {
(read_data_file(path)?, format!("`{path}`"))
};
- // An empty body reaches `attach_body` as invalid JSON and is reported as
- // one — "EOF while parsing a value" — which describes the symptom and not
- // the mistake. The mistake is almost always a pipe that produced nothing
- // (`cat missing.json | mapbox …`, whose own error went to the same stderr
- // and scrolled past), and naming the source is what points at it.
+ // An empty body would otherwise reach `attach_body` as invalid JSON —
+ // "EOF while parsing a value" — describing the symptom, not the
+ // mistake. The real mistake is almost always an empty pipe (`cat
+ // missing.json | mapbox …`, whose own error scrolled past on the same
+ // stderr), so naming the source here points at the actual cause.
if body.trim().is_empty() {
return Err(CliError::new(
"invalid_data",
@@ -893,10 +887,10 @@ fn resolve_data(value: &str) -> Result> {
/// A `--data @path` file, as text.
///
-/// Text rather than bytes, and that is a check rather than a convenience: a
-/// JSON body has to be UTF-8, so a file that is not says so here instead of
-/// being lossily converted into a body the API would reject for reasons that
-/// name nothing the caller did.
+/// Text, not bytes — that's a validity check, not just convenience. A
+/// JSON body has to be UTF-8, so a file that isn't gets reported here
+/// instead of silently lossy-converted into a body the API would reject
+/// for reasons that don't point back at the actual mistake.
fn read_data_file(path: &str) -> Result {
std::fs::read_to_string(path).map_err(|e| {
let message = if e.kind() == std::io::ErrorKind::InvalidData {
@@ -928,8 +922,8 @@ fn read_stdin() -> Result {
/// Picks between `--data` and `--file` for an operation that takes a body.
///
-/// Pure, and kept that way: every rejection here is a mistake the caller can
-/// fix from the message alone, without a request having been sent.
+/// Kept pure: every rejection here is a mistake the caller can fix from
+/// the message alone, with no request ever sent.
fn resolve_body_source<'a>(
body: &'a RequestBody,
data: Option<&'a str>,
@@ -973,8 +967,9 @@ fn resolve_body_source<'a>(
});
}
- // A raw body is one file by definition. clap keeps only the last
- // occurrence for this operation, so this guards the function, not the CLI.
+ // A raw body is one file by definition. clap already keeps only the
+ // last `--file` for this operation, so this guards the function
+ // itself, not the CLI's parsing.
if files.len() > 1 {
return Err(CliError::new(
"conflicting_body",
@@ -992,21 +987,22 @@ fn resolve_body_source<'a>(
})
}
-/// How much the request is about to move, which is all the budget turns on.
+/// How much data the request is about to send — all the timeout budget
+/// depends on.
///
-/// `--file` is unbounded, and so is a `--data @path` or `--data @-`, which is
-/// why this takes a second argument rather than reading the body alone. A
-/// `--data` body *typed* on a command line is capped by argv at a megabyte or
-/// so and goes out inside the ordinary budget with room to spare — that was
-/// once true of every `--data` body, and the reasoning is the thing `@path`
-/// broke: `BodySource::Json` looks identical whether it was typed or read from
-/// a 200 MB file, and the second would have been given a sixty-second budget
-/// it could not meet.
+/// `--file` is unbounded, and so is `--data @path` or `--data @-`, which is
+/// why this needs a second argument instead of just reading the body. A
+/// `--data` body *typed* on the command line is capped by argv at roughly
+/// a megabyte and fits the ordinary budget with room to spare — that used
+/// to be true of every `--data` body, until `@path` broke it:
+/// `BodySource::Json` looks identical whether it was typed or read from a
+/// 200 MB file, and the second case would get a sixty-second budget it
+/// could never meet.
///
-/// The response is not consulted, because nothing here knows it yet: six of
-/// the twelve services answer with bytes, but a tile, a glyph range and a
-/// style ZIP all arrive well inside a minute, so the one shape worth
-/// separating out is the one this CLI is sending.
+/// The response isn't consulted, since nothing here has seen it yet — and
+/// it wouldn't help anyway: even the six of twelve services that answer
+/// with bytes (tiles, glyph ranges, style ZIPs) arrive well inside a
+/// minute. Only what this CLI is *sending* is worth budgeting for.
fn payload_of(body: Option<&BodySource<'_>>, data_was_read: bool) -> http::Payload {
match body {
Some(BodySource::Raw { .. } | BodySource::Multipart { .. }) => http::Payload::File,
@@ -1052,8 +1048,8 @@ fn attach_body(
/// A file named by `--file`, as bytes.
///
-/// A path that does not exist is the most likely thing to go wrong with this
-/// flag, and it must read as the caller's typo rather than as a crash.
+/// A missing path is the most likely thing to go wrong here, and it needs
+/// to read as the caller's typo, not as a crash.
fn read_body_file(path: &str) -> Result> {
std::fs::read(path).map_err(|e| {
CliError::new(
@@ -1066,8 +1062,8 @@ fn read_body_file(path: &str) -> Result> {
/// The last path segment, which is what a multipart part is named by.
///
-/// It is not cosmetic for sprites: `batchUploadSprite` takes the icon name
-/// from each part's filename, so `zz-clitest-1.svg` becomes the icon
+/// Not cosmetic for sprites: `batchUploadSprite` takes the icon name from
+/// each part's filename, so `zz-clitest-1.svg` becomes the icon
/// `zz-clitest-1`.
fn file_name_of(path: &str) -> &str {
path.rsplit(['/', '\\']).next().unwrap_or(path)
@@ -1075,9 +1071,9 @@ fn file_name_of(path: &str) -> &str {
/// The media type to label one multipart part with.
///
-/// The spec describes the parts as `format: binary` and nothing more, so the
-/// extension is the only evidence available. Getting it wrong matters:
-/// `batchUploadSprite` rejects a part that does not claim to be SVG.
+/// The spec only describes these parts as `format: binary`, so the file
+/// extension is the only evidence we have. Getting it wrong matters:
+/// `batchUploadSprite` rejects a part that doesn't claim to be SVG.
fn part_media_type(path: &str) -> &'static str {
let extension = file_name_of(path)
.rsplit_once('.')
@@ -1093,12 +1089,11 @@ fn part_media_type(path: &str) -> &'static str {
/// What to say when a successful response carries no body.
///
-/// The HTTP method is all there is to go on: these commands are generated
-/// from the specs and share one code path, so a 204 looks the same whether a
-/// style was deleted or a folder renamed. Naming the resource makes the line
-/// specific enough to be worth reading, and the last path parameter is the
-/// one that identifies it — the earlier ones scope it (`{username}`, then
-/// `{style_id}`, then `{icon_name}`).
+/// The HTTP method is all we have to go on: generated commands share one
+/// code path, so a 204 looks the same whether a style was deleted or a
+/// folder renamed. Naming the resource makes the line worth reading — the
+/// last path parameter identifies it, while the earlier ones just scope it
+/// (`{username}`, then `{style_id}`, then `{icon_name}`).
fn empty_success_text(op: &Operation, matches: &ArgMatches) -> String {
let subject = op
.path_params
@@ -1116,58 +1111,56 @@ fn empty_success_line(method: &str, subject: Option<&str>) -> String {
match method.to_ascii_uppercase().as_str() {
"DELETE" => format!("Deleted{subject}."),
"POST" | "PUT" | "PATCH" => format!("Done{subject}."),
- // A GET that answers 200 with an empty body: the session endpoints.
- // Nothing was changed, so nothing is claimed.
+ // A GET answering 200 with an empty body — the session endpoints.
+ // Nothing changed, so nothing is claimed.
_ => "No content.".to_string(),
}
}
/// Put one path parameter's value into the template.
///
-/// An optional parameter that occupies a whole segment — `/{overlay}` is
-/// the only one any invocation can now reach — has to take its slash with it
-/// when the value is empty.
-/// Substituting an empty string in place leaves `//`, which the API reads as
-/// a segment that is present and empty and answers 404, so asking for a plain
-/// map image with no overlay would fail while the same URL without the
-/// segment returns it.
+/// An optional parameter that fills a whole segment (`/{overlay}` is the
+/// only one any invocation can reach today) has to take its slash with it
+/// when the value is empty. Substituting an empty string in place would
+/// leave `//`, which the API reads as an empty-but-present segment and
+/// answers 404 to — so a plain map image with no overlay would fail, while
+/// the same URL without the segment succeeds.
///
/// Parameters that are only part of a segment (`{width}x{height}{format}`)
-/// are substituted as they are: an empty value there is the format's default,
-/// which is what the spec means by optional.
-/// The characters that would change the URL's *structure* rather than name a
-/// segment within it.
+/// get substituted as-is: an empty value there is just the format's
+/// default, which is what "optional" means for them.
///
-/// `\` is here because WHATWG treats it as a path separator for special
-/// schemes, so `..\..\x` traverses exactly as `../../x` does — verified
-/// against `reqwest::Url`, not assumed.
+/// `\` is included because WHATWG treats it as a path separator for
+/// special schemes, so `..\..\x` traverses exactly like `../../x` —
+/// verified against `reqwest::Url`, not assumed.
const PATH_STRUCTURAL: [char; 4] = ['/', '?', '#', '\\'];
/// One path parameter's value, safe to splice into the URL's path.
///
/// The path is built by substituting into a template
/// (`…/{username}/{style_id}/static/{overlay}/…`), so a value carrying URL
-/// syntax used to change which request went out. With the caller's token and
-/// the command's method attached, `styles delete '../../x'` aimed a `DELETE`
-/// at a path nobody asked for, and `'x?fresh=true'` appended a query
-/// parameter — the same shape as mapbox/mcp-server's `directions_tool` fix.
+/// syntax used to be able to change which request went out. With the
+/// caller's token and the command's method attached, `styles delete
+/// '../../x'` aimed a `DELETE` at a path nobody asked for, and
+/// `'x?extra=1'` appended a query parameter — the same shape as
+/// mapbox/mcp-server's `directions_tool` fix.
///
-/// **Only the four structural characters are encoded, deliberately.** Path
-/// parameters here carry punctuation on purpose: a static-images overlay is
-/// `pin-s+f74e4e(-122.46,37.77)`, `{highRes}` is `@2x`, `{format}` is `.png`,
-/// and `{lon},{lat},{zoom}` are three placeholders sharing one comma-
-/// separated segment. Percent-encoding everything outside RFC 3986's
-/// unreserved set would rewrite all of that and risk breaking requests that
-/// work today. Encoding only what alters the URL's shape cannot change any
-/// request that does not already contain those four characters.
+/// **Only these four structural characters are encoded, deliberately.**
+/// Path parameters here carry punctuation on purpose: a static-images
+/// overlay is `pin-s+f74e4e(-122.46,37.77)`, `{highRes}` is `@2x`,
+/// `{format}` is `.png`, and `{lon},{lat},{zoom}` share one comma-separated
+/// segment. Percent-encoding everything outside RFC 3986's unreserved set
+/// would rewrite all of that and risk breaking requests that work today.
+/// Encoding only the characters that change the URL's shape can't break
+/// any request that doesn't already contain one of those four.
///
-/// Dot segments are refused rather than encoded, because encoding does not
-/// stop them: WHATWG reads `%2e%2e` as a double-dot segment too, so a value
-/// of exactly `..` still climbs a level however it is spelled. Encoding the
-/// separators is what defeats the multi-level `../../x` case — it collapses
-/// to a single segment — and this catches the single-level remainder.
+/// Dot segments are refused, not encoded, because encoding doesn't stop
+/// them — WHATWG reads `%2e%2e` as a double-dot segment too, so a value of
+/// exactly `..` still climbs a level no matter how it's spelled. Encoding
+/// the separators handles the multi-level `../../x` case (it collapses to
+/// one segment); this handles the single-level case that's left over.
fn path_segment<'a>(name: &str, value: &'a str) -> Result> {
- // `%2e` is a dot as far as the URL parser is concerned, in either case.
+ // The URL parser treats `%2e` (either case) as a plain dot.
let as_dots = value.replace("%2e", ".").replace("%2E", ".");
if as_dots == "." || as_dots == ".." {
return Err(CliError::new(
@@ -1208,27 +1201,25 @@ fn substitute_path_param(path: &str, name: &str, value: &str, required: bool) ->
/// A transport failure, with the URL stripped out of it.
///
-/// `reqwest::Error`'s `Display` appends " for url (…)", and the access token
-/// travels in the query string — so the default rendering puts a live token
-/// into an error message, which under `--output json` is a field that agents,
-/// CI logs and pasted issue reports all capture verbatim. The `--debug` path
-/// above already redacts the token for exactly this reason.
+/// `reqwest::Error`'s `Display` appends " for url (…)", and the access
+/// token travels in the query string — so the default rendering puts a
+/// live token into an error message that `--output json`, CI logs, and
+/// pasted issue reports would all capture verbatim. The `--debug` path
+/// already redacts the token for this same reason.
///
-/// Stripping the URL costs the only detail reqwest's outermost layer carried,
-/// so the source chain is walked to recover what actually went wrong —
-/// otherwise every proxy, DNS, TLS and timeout failure reads "error sending
-/// request" and nothing more.
+/// Stripping the URL loses the only detail reqwest's outermost layer
+/// carried, so we walk the source chain to recover what actually went
+/// wrong — otherwise every proxy, DNS, TLS, and timeout failure would just
+/// read "error sending request" and nothing more.
///
-/// A timeout is then told apart from the rest and given its own code, because
-/// it is the one of them a caller can do something about from here: the
-/// budget it ran out of is one `--timeout` moves. Sending that reader to
-/// check their proxy, which is what the generic advice does, points away from
-/// the answer.
+/// A timeout gets its own error code, since it's the one failure here a
+/// caller can actually act on: raise the `--timeout` budget. Generic
+/// "check your proxy" advice would point them the wrong way.
///
-/// Shared with `auth`'s `--verify`, which reaches an endpoint that takes the
-/// token as a query parameter too. One redaction, not two.
+/// Shared with `auth`'s `--verify`, which also reaches an endpoint that
+/// takes the token as a query parameter — one redaction covers both.
pub(crate) fn transport_failure(context: &str, err: reqwest::Error) -> CliError {
- // Asked before `without_url`, which builds a new error out of this one.
+ // Checked before `without_url`, which builds a new error from this one.
let ran_out_of_time = err.is_timeout();
let err = err.without_url();
let mut message = format!("{context}: {err}");
@@ -1240,10 +1231,10 @@ pub(crate) fn transport_failure(context: &str, err: reqwest::Error) -> CliError
}
if ran_out_of_time {
- // Its own code, not a variant of the message: this is the one
- // transport failure a script has a sensible thing to do about, which
- // is to retry it or to raise the budget. Reading prose to find that
- // out is not a contract.
+ // Its own error code, not just a message variant: this is the one
+ // transport failure a script can sensibly react to (retry, or
+ // raise the budget), and a script shouldn't have to parse prose to
+ // find that out.
return CliError::new("request_timed_out", message).with_remedy(remedy::for_timeout());
}
@@ -1253,10 +1244,10 @@ pub(crate) fn transport_failure(context: &str, err: reqwest::Error) -> CliError
/// Points a rejected argument at `--schema`, which describes what the
/// command would have accepted.
///
-/// Only for the codes it answers, since this sees every failure `execute`
-/// produces: an unreadable `--file` path is a filesystem problem and a 404
-/// is not an argument problem at all, so offering a schema for either would
-/// be advice that does not apply.
+/// Only reacts to the codes listed below, since this sees every failure
+/// `execute` produces: an unreadable `--file` is a filesystem problem, and
+/// a 404 isn't an argument problem at all, so offering a schema for either
+/// would be advice that doesn't apply.
fn with_schema_action(err: anyhow::Error, op: &Operation) -> anyhow::Error {
const SCHEMA_ANSWERS: [&str; 3] = ["invalid_data", "conflicting_body", "unsupported_body"];
@@ -1272,9 +1263,9 @@ fn with_schema_action(err: anyhow::Error, op: &Operation) -> anyhow::Error {
/// "To see one of these, run …", when the spec describes such a command.
///
/// Built from the operation the caller actually ran, so it can only ever
-/// name a command that exists — `accounts list-tokens` gets nothing, because
-/// the Tokens API has no way to fetch one token by id, and inventing a
-/// plausible-looking suggestion is worse than staying quiet.
+/// name a command that exists. `accounts list-tokens` gets nothing, since
+/// the Tokens API has no way to fetch one token by id — inventing a
+/// plausible-looking suggestion here would be worse than staying quiet.
fn detail_hint(op: &Operation) -> Option {
let detail = op.detail.as_ref()?;
Some(format!("mapbox {} <{}>", detail.command, detail.parameter))
@@ -1311,11 +1302,11 @@ fn is_binary_content_type(content_type: &str) -> bool {
)
}
-/// The file extension to suggest for a response we are refusing to print.
+/// The file extension to suggest for a response we're refusing to print.
///
-/// Guessing from the content type rather than always saying `.png`: telling
-/// someone to redirect a glyph range into `out.png` is advice that produces
-/// a mislabeled file, and it reads as though the command misunderstood what
+/// Guessed from the content type instead of always saying `.png` — telling
+/// someone to redirect a glyph range into `out.png` produces a
+/// mislabeled file and makes it look like the command misunderstood what
/// it fetched.
fn suggested_extension(content_type: &str) -> &'static str {
let essence = content_type
@@ -1347,9 +1338,9 @@ fn suggested_extension(content_type: &str) -> &'static str {
}
}
-/// Writes raw bytes to stdout, refusing to do so when that is a terminal —
-/// `curl`'s behavior, and for the same reason: a few hundred KB of PNG will
-/// otherwise scramble the user's shell.
+/// Writes raw bytes to stdout, refusing to do so when that's a terminal —
+/// same as `curl`, and for the same reason: a few hundred KB of PNG would
+/// otherwise scramble the shell.
fn write_binary(body: &[u8], content_type: &str) -> Result<()> {
use std::io::{IsTerminal, Write};
@@ -1392,16 +1383,16 @@ mod tests {
use crate::output::CliError;
use crate::spec::{Parameter, RequestBody};
- /// The `CliError` inside a refusal, so a test can name the code the
- /// caller would see rather than match on prose.
+ /// The `CliError` inside a refusal, so a test can check the error code
+ /// instead of matching on prose.
fn refusal(err: anyhow::Error) -> CliError {
err.downcast::().expect("refused with a CliError")
}
fn body(content_types: &[&str], multipart_field: Option<&str>) -> RequestBody {
RequestBody {
- // Nothing here reads it: `resolve_body_source` decides from the
- // flags it was given, not from whether the spec insists on one.
+ // `resolve_body_source` decides from the flags, not from
+ // whether the spec insists on a body — so this doesn't matter.
required: false,
content_types: content_types.iter().map(|s| s.to_string()).collect(),
multipart_field: multipart_field.map(|s| s.to_string()),
@@ -1417,8 +1408,6 @@ mod tests {
("application/vnd.mapbox-vector-tile", ".mvt"),
("application/zip", ".zip"),
("font/woff2", ".woff2"),
- // Parameters and casing must not change the answer, and an
- // unknown type still has to name something.
("IMAGE/PNG; charset=binary", ".png"),
("application/octet-stream", ".bin"),
("", ".bin"),
@@ -1485,8 +1474,8 @@ mod tests {
#[test]
fn an_empty_value_inside_a_segment_stays_empty() {
- // `{highRes}` and `{format}` are the segment's optional tail: empty
- // means the default, not a segment to remove.
+ // `{highRes}`/`{format}` are part of a segment, not a whole one —
+ // empty means the format's default, not something to remove.
let template = "/styles/v1/user/s/tiles/512/1/2/3{highRes}{format}";
assert_eq!(
substitute_path_param(template, "highRes", "", false),
@@ -1496,8 +1485,8 @@ mod tests {
#[test]
fn a_required_segment_is_left_empty_rather_than_dropped() {
- // Dropping it would send a URL that means something else. The empty
- // segment reaches the API and it says what is wrong.
+ // Dropping it would send a URL that means something else; leaving
+ // it empty reaches the API, which can say what's actually wrong.
let template = "/v4/{tilesets}/1/2/3.png";
assert_eq!(
substitute_path_param(template, "tilesets", "", true),
@@ -1505,9 +1494,9 @@ mod tests {
);
}
- /// `starFile`'s body is the word `true`, under `text/plain`. It goes
- /// out as typed: parsing it as JSON and re-encoding would be a
- /// round trip through a format the endpoint rejects.
+ /// `starFile`'s body is the literal word `true`, under `text/plain`.
+ /// It's sent as typed — parsing it as JSON and re-encoding would round
+ /// it through a format the endpoint rejects.
#[test]
fn a_text_body_is_sent_as_typed() {
let plain = body(&["text/plain"], None);
@@ -1520,7 +1509,6 @@ mod tests {
);
}
- /// The JSON operations are untouched by that.
#[test]
fn a_json_body_still_goes_through_data() {
let json = body(&["application/json"], None);
@@ -1530,8 +1518,8 @@ mod tests {
);
}
- /// A 204 says only that it worked. The method is what makes the line
- /// specific, and an empty body must never read as an empty result.
+ /// A 204 only says that it worked; the HTTP method is what makes the
+ /// line specific.
#[test]
fn an_empty_success_says_what_happened() {
assert_eq!(
@@ -1550,17 +1538,17 @@ mod tests {
assert_eq!(empty_success_line("PUT", None), "Done.");
}
- /// The session endpoints answer 200 with nothing in the body. They change
- /// no state, so the line must not claim they did.
+ /// The session endpoints answer 200 with an empty body but change no
+ /// state, so the line must not claim they did.
#[test]
fn an_empty_get_claims_nothing() {
assert_eq!(empty_success_line("GET", None), "No content.");
assert_eq!(empty_success_line("GET", Some("ignored")), "No content.");
}
- /// The spec's media type has to reach the wire verbatim. Sending SVG as
- /// `application/json` is exactly the bug that made `upload-sprite-image`
- /// unusable, and it looked like a working command the whole time.
+ /// The spec's media type must reach the wire verbatim. Sending SVG as
+ /// `application/json` is exactly the bug that made
+ /// `upload-sprite-image` unusable while still looking like it worked.
#[test]
fn a_raw_body_is_sent_as_the_type_the_spec_declared() {
let svg = body(&["image/svg+xml"], None);
@@ -1582,8 +1570,6 @@ mod tests {
);
}
- /// The field name comes from the spec, so a spec that renames it does not
- /// need this code changed.
#[test]
fn multipart_files_go_under_the_field_the_spec_names() {
let form = body(&["multipart/form-data"], Some("images"));
@@ -1605,8 +1591,9 @@ mod tests {
);
}
- /// An operation that takes a body but was given nothing still sends the
- /// request: whether the body was required is the API's answer to give.
+ /// An operation that takes a body but got none still sends the
+ /// request — whether the body was actually required is the API's
+ /// answer to give, not ours.
#[test]
fn no_flag_at_all_sends_an_empty_body() {
let json = body(&["application/json"], None);
@@ -1616,8 +1603,8 @@ mod tests {
);
}
- /// Both flags set one body between them, so taking either silently would
- /// discard what the caller asked for.
+ /// Picking one flag silently would discard what the caller asked for
+ /// with the other.
#[test]
fn data_and_file_together_are_refused() {
let both = body(&["application/octet-stream", "application/json"], None);
@@ -1627,8 +1614,8 @@ mod tests {
assert!(err.message.contains("--file"), "{}", err.message);
}
- /// `--file` is never offered on a JSON-only operation, but the resolver
- /// must not depend on clap having enforced that.
+ /// `--file` is never offered on a JSON-only operation, but the
+ /// resolver still shouldn't depend on clap having enforced that.
#[test]
fn a_file_for_a_json_only_body_names_the_flag_that_works() {
let json = body(&["application/json"], None);
@@ -1644,8 +1631,8 @@ mod tests {
assert_eq!(err.code, "conflicting_body");
}
- /// `initUpload` declares octet-stream *and* JSON. The first non-JSON type
- /// is what `--file` means; `--data` keeps the JSON half.
+ /// `initUpload` declares octet-stream *and* JSON: the non-JSON type is
+ /// what `--file` sends, and `--data` still sends the JSON half.
#[test]
fn a_body_declaring_both_supports_each_flag_on_its_own() {
let both = body(&["application/octet-stream", "application/json"], None);
@@ -1663,7 +1650,7 @@ mod tests {
}
/// The sprite API names each icon after its part's filename, so the
- /// directories in front of it must not travel with it.
+ /// leading directories can't come along.
#[test]
fn a_part_is_named_by_the_file_not_its_path() {
assert_eq!(
@@ -1674,9 +1661,6 @@ mod tests {
assert_eq!(file_name_of("./a/b/c.svg"), "c.svg");
}
- /// The spec calls every part `format: binary` and stops there, so the
- /// extension is the only evidence — and `batchUploadSprite` rejects a
- /// part that does not claim to be SVG.
#[test]
fn a_parts_media_type_comes_from_its_extension() {
assert_eq!(part_media_type("icon.svg"), "image/svg+xml");
@@ -1689,9 +1673,9 @@ mod tests {
);
}
- /// The token travels in the query string, so the one function that
- /// renders this URL is the one thing standing between a live credential
- /// and `--debug`'s stderr or a dry run's stdout.
+ /// The token travels in the query string, so this rendering function
+ /// is all that stands between a live credential and `--debug`'s
+ /// stderr or a dry run's stdout.
#[test]
fn the_rendered_url_never_carries_the_token() {
let query = [
@@ -1785,8 +1769,8 @@ mod tests {
assert_eq!(value, "coffee&limit=99&access_token=sk.theirs");
}
- /// An unauthenticated call has no query at all, and a URL ending in `?`
- /// is not the request that would be sent.
+ /// An unauthenticated call has no query at all, so a trailing `?`
+ /// would misrepresent the request.
#[test]
fn a_url_with_no_query_keeps_no_question_mark() {
assert_eq!(
@@ -1795,19 +1779,17 @@ mod tests {
);
}
- /// The dry run's whole promise is that what it accepts, the real call
- /// would send. A body it declined to parse breaks that in the direction
- /// that costs the most: a `--dry-run` that passes and a `create` that
- /// then fails on the same argument.
+ /// A dry run promises that what it accepts, a real call would send —
+ /// so it must reject the same invalid JSON the real send would, not
+ /// let it pass and fail later.
#[test]
fn a_dry_run_rejects_the_json_that_sending_would_reject() {
let err = describe_body(&BodySource::Json("{oops")).expect_err("invalid JSON is refused");
assert_eq!(refusal(err).code, "invalid_data");
}
- /// And a `--file` that is not there. Reading rather than stat-ing is what
- /// makes this catch an unreadable file too, which a metadata check would
- /// wave through.
+ /// Reading the file (not just stat-ing it) is what catches this —
+ /// a metadata check would wave an unreadable file through.
#[test]
fn a_dry_run_rejects_a_file_that_cannot_be_read() {
let err = describe_body(&BodySource::Raw {
@@ -1818,8 +1800,7 @@ mod tests {
assert_eq!(refusal(err).code, "invalid_file");
}
- /// An operation whose body the caller left out has nothing to describe,
- /// and a "Body:" line saying so would be noise on every `delete`.
+ /// A "Body:" line on every `delete` would just be noise.
#[test]
fn a_body_that_was_never_given_is_not_described() {
assert!(describe_body(&BodySource::Empty)
@@ -1827,14 +1808,13 @@ mod tests {
.is_none());
}
- /// `batchUploadSprite` takes each icon's name from its part's filename,
- /// so the plan has to name the icons the upload would create — the path
- /// alone leaves the reader to work out that `icons/foo.svg` becomes
- /// `foo.svg`.
+ /// `batchUploadSprite` takes each icon's name from its filename, so
+ /// the plan should spell that out rather than leave the reader to
+ /// work out that `icons/foo.svg` becomes `foo.svg`.
#[test]
fn a_multipart_plan_names_each_part() {
- // `CARGO_TARGET_TMPDIR` is an integration-test variable and does not
- // exist here; the pid keeps two concurrent runs on one CI box apart.
+ // `CARGO_TARGET_TMPDIR` isn't set here (it's an integration-test
+ // variable); the pid keeps concurrent runs on one CI box apart.
let dir = std::env::temp_dir().join(format!("mapbox-cli-dry-run-{}", std::process::id()));
std::fs::create_dir_all(&dir).expect("create temp dir");
let icon = dir.join("zz-clitest-1.svg");
@@ -1861,18 +1841,14 @@ mod tests {
std::fs::remove_dir_all(&dir).expect("clean up temp dir");
}
- /// A budget that runs out is reported as that, not as a network to go and
- /// check.
- ///
- /// Built from a real `reqwest` timeout rather than a hand-made error,
- /// because what is being pinned is that `is_timeout()` still answers true
- /// after the whole `transport_failure` path has taken the URL out of the
- /// error — the token rides in that URL, so the stripping is not optional
- /// and the classification has to survive it.
+ /// Uses a real `reqwest` timeout instead of a hand-made error, because
+ /// what's being tested is that `is_timeout()` still answers true after
+ /// `transport_failure` strips the URL out — the token rides in that
+ /// URL, so the stripping must not break the classification.
#[test]
fn a_budget_that_runs_out_says_so_rather_than_blaming_the_network() {
- // Accepts and never answers, and is held open: dropped, the port
- // closes and the client gets a refusal instead of a silence.
+ // Accepts and never answers, and stays held open — dropped, the
+ // port closes and the client gets a refusal instead of a hang.
let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("a loopback port");
let addr = listener.local_addr().expect("the bound address");
@@ -1897,12 +1873,9 @@ mod tests {
);
}
- /// Which requests get the longer budget, and — the half that is easy to
- /// get wrong — which do not.
- ///
/// A `--data` body is bounded by what a command line can carry, so
- /// putting fifteen minutes in front of a `create-style` would only mean
- /// waiting a quarter of an hour to be told the API is down.
+ /// giving `create-style` a fifteen-minute budget would only mean
+ /// waiting that long to find out the API is down.
#[test]
fn only_a_file_is_treated_as_a_transfer() {
let raw = BodySource::Raw {
@@ -1932,10 +1905,10 @@ mod tests {
assert_eq!(payload_of(Some(&text), false), Payload::Bounded);
}
- /// A body read from `@path` or `@-` is indistinguishable from a typed one
- /// by the time it reaches `BodySource::Json`, and nothing bounds its size.
- /// Given the sixty-second budget, a large one would fail on a timeout that
- /// described the network rather than the choice of flag.
+ /// A body read from `@path`/`@-` looks identical to a typed one by the
+ /// time it reaches `BodySource::Json`, and nothing bounds its size. On
+ /// the ordinary sixty-second budget, a large one would time out in a
+ /// way that blames the network instead of the choice of flag.
#[test]
fn a_data_body_that_was_read_gets_the_transfer_budget() {
assert_eq!(
@@ -1968,9 +1941,9 @@ mod tests {
assert!(resolved.streamed, "nothing bounds a file");
}
- /// The trailing newline a text editor leaves is *not* stripped. It is
- /// insignificant to every JSON parser, and trimming a body the caller
- /// supplied would be this CLI quietly editing what it was asked to send.
+ /// A trailing newline a text editor leaves is *not* stripped — every
+ /// JSON parser ignores it anyway, and trimming it would mean this CLI
+ /// quietly editing what the caller asked to send.
#[test]
fn a_read_body_is_sent_byte_for_byte() {
let dir = tempdir();
@@ -1993,9 +1966,9 @@ mod tests {
);
}
- /// A JSON body has to be UTF-8, so this is a check rather than a
- /// convenience — the alternative is a lossy conversion the API rejects for
- /// reasons that name nothing the caller did.
+ /// A JSON body has to be UTF-8, so this is a validity check, not just
+ /// a convenience — the alternative is a lossy conversion the API
+ /// would then reject for reasons that don't point back at the mistake.
#[test]
fn a_non_utf8_file_says_so_rather_than_being_mangled() {
let dir = tempdir();
@@ -2008,9 +1981,9 @@ mod tests {
assert!(cli.message.contains("not valid UTF-8"), "{}", cli.message);
}
- /// The mistake is almost always a pipe that produced nothing, and the
- /// symptom without this is "EOF while parsing a value", which names the
- /// parser rather than the pipe.
+ /// Without this, the symptom is "EOF while parsing a value" — naming
+ /// the parser instead of the empty pipe that's almost always the real
+ /// cause.
#[test]
fn an_empty_file_says_which_source_was_empty() {
let dir = tempdir();
@@ -2032,8 +2005,8 @@ mod tests {
assert!(cli.message.contains("@-"), "{}", cli.message);
}
- /// A body that merely *contains* an `@` is not a path. Only the first
- /// character decides, which is what makes `--data '{"a":"b@c"}'` safe.
+ /// Only the first character decides whether this is a path — which is
+ /// what makes `--data '{"a":"b@c"}'` safe.
#[test]
fn an_at_sign_inside_the_body_is_not_a_path() {
let resolved = resolve_data(r#"{"email":"a@b.example"}"#).expect("a literal body");
@@ -2041,8 +2014,8 @@ mod tests {
assert_eq!(resolved.body, r#"{"email":"a@b.example"}"#);
}
- /// A scratch directory that cleans itself up, so these tests leave
- /// nothing behind and cannot collide with each other.
+ /// A scratch directory that cleans itself up, so tests leave nothing
+ /// behind and can't collide with each other.
fn tempdir() -> TempDir {
let base = std::env::temp_dir().join(format!(
"mapbox-cli-data-{}-{:?}",
@@ -2091,26 +2064,26 @@ mod tests {
assert!(tip.contains("--limit 10"), "{tip}");
}
- /// A parameter the API sent back but this command does not declare has no
- /// flag to name, so it is left out rather than invented.
+ /// A parameter the API sent back but this command doesn't declare has
+ /// no flag to name, so it's left out instead of invented.
#[test]
fn an_undeclared_query_parameter_is_not_named() {
let declared = [param("start")];
- let next = "https://api.mapbox.com/a?start=7&fresh=true";
+ let next = "https://api.mapbox.com/a?start=7&extra=1";
let tip = NextPage::of(&declared, next).tip();
assert!(tip.contains("--start 7"), "{tip}");
- assert!(!tip.contains("fresh"), "{tip}");
+ assert!(!tip.contains("extra"), "{tip}");
}
/// **The security property of this whole path.**
///
- /// The access token rides in the query string, so the URL the API echoes
- /// back in `Link` contains a live token. It is excluded by construction —
- /// `dispatch` adds it to the query directly rather than declaring it in
- /// `op.query_params`, and only declared parameters become flags — but
- /// "by construction" is worth a test, because the cost of being wrong is
- /// printing a credential to a terminal and into whatever captured it.
+ /// The access token rides in the query string, so the `Link` URL the
+ /// API echoes back contains a live token. It's excluded by
+ /// construction — `dispatch` adds it directly rather than declaring it
+ /// in `op.query_params`, and only declared parameters become flags —
+ /// but that's worth testing directly, since being wrong here means
+ /// printing a credential to a terminal (and to whatever captured it).
#[test]
fn the_page_tip_never_names_the_access_token() {
let secret = "pk.eyJ1IjoibWFwYm94IiwiYSI6ImNqa2xpdmV0b2tlbiJ9.aaaaaaaaaaaaaaaaaaaaaa";
@@ -2126,9 +2099,9 @@ mod tests {
}
}
- /// Even if someone later declares a parameter by that name, which is the
- /// way the guarantee above could be undone from a spec rather than from
- /// this file.
+ /// Even if a spec later declares a parameter literally named
+ /// `access_token` — the way the guarantee above could be undone from
+ /// the spec side instead of this file.
#[test]
fn a_declared_parameter_named_access_token_is_still_withheld() {
let declared = [param(ACCESS_TOKEN), param("start")];
@@ -2139,9 +2112,8 @@ mod tests {
assert!(tip.contains("--start 3"), "{tip}");
}
- /// The values are decoded, because the CLI re-encodes whatever it is
- /// given: handing back `%2B` would round-trip to `%252B` and fetch the
- /// wrong page.
+ /// Decoded because the CLI re-encodes whatever it's given: handing
+ /// back `%2B` would round-trip to `%252B` and fetch the wrong page.
#[test]
fn the_page_tip_decodes_percent_escapes() {
let declared = [param("start")];
@@ -2151,8 +2123,8 @@ mod tests {
assert!(tip.contains("--start a+b"), "{tip}");
}
- /// A value with a space in it would silently become two arguments if the
- /// tip were pasted unquoted.
+ /// A value with a space in it would silently become two arguments if
+ /// pasted unquoted.
#[test]
fn a_value_needing_a_shell_quote_gets_one() {
assert_eq!(shell_value("cjk2ab"), "cjk2ab");
@@ -2162,9 +2134,8 @@ mod tests {
assert_eq!(shell_value("it's"), r"'it'\''s'");
}
- /// An operation the API pages but whose spec declares no paging
- /// parameter. The result is incomplete either way, so saying so beats
- /// saying nothing — but it must not claim a flag that does not exist.
+ /// The result is incomplete either way, so this should still say so —
+ /// without claiming a flag that doesn't exist.
#[test]
fn no_declared_paging_parameter_still_says_the_result_is_partial() {
let tip = NextPage::of(&[param("unrelated")], "https://api.mapbox.com/a?start=3").tip();
@@ -2172,8 +2143,8 @@ mod tests {
assert!(!tip.contains("--"), "{tip}");
}
- /// An unparseable `Link` target costs nothing: the request succeeded, and
- /// a tip is not worth turning that into a failure.
+ /// An unparseable `Link` target costs nothing — the request succeeded,
+ /// and a tip isn't worth turning that into a failure.
#[test]
fn an_unparseable_next_url_yields_no_flags() {
assert!(query_pairs("not a url").is_empty());
@@ -2204,12 +2175,12 @@ mod tests {
/// The header that actually arrives in practice.
///
- /// No Mapbox endpoint reachable from here sends `x-request-id` — styles,
- /// tokens, fonts and geocoding v6 were all checked, on success and on a
- /// 404. Every one of them sends `x-amz-cf-id`, because the API is fronted
- /// by CloudFront. Looking for the conventional name alone would have made
- /// this feature inert, which is what this test exists to stop happening
- /// again.
+ /// No Mapbox endpoint reachable from here sends `x-request-id` —
+ /// styles, tokens, fonts, and geocoding v6 were all checked, on
+ /// success and on a 404. All of them send `x-amz-cf-id` instead, since
+ /// the API sits behind CloudFront. Checking only the conventional name
+ /// would make this whole feature inert, which is what this test guards
+ /// against.
#[test]
fn the_cloudfront_id_is_read_when_there_is_no_request_id() {
let mut map = reqwest::header::HeaderMap::new();
@@ -2224,8 +2195,8 @@ mod tests {
);
}
- /// A service that sends its own id means it more specifically than the
- /// CDN in front of it does.
+ /// A service that sends its own id is more specific than the CDN in
+ /// front of it.
#[test]
fn an_explicit_request_id_outranks_the_cloudfront_one() {
let mut map = reqwest::header::HeaderMap::new();
@@ -2246,7 +2217,7 @@ mod tests {
assert_eq!(request_id(&map), None);
}
- /// A header present but blank says nothing, and an empty request id would
+ /// A present-but-blank header says nothing; an empty request id would
/// print as `Request ID:` with nothing after it.
#[test]
fn a_blank_header_reads_as_absent() {
@@ -2272,10 +2243,9 @@ mod tests {
assert_eq!(ResponseHeaders::read(&map).next_page, None);
}
- /// `--id` searches the page it was handed. On a paginated response
- /// "No row has the id" is true of the page and may be false of the
- /// listing, which is the most misleading form of the truncation this
- /// path exists to stop.
+ /// On a paginated response, "No row has the id" is true of this page
+ /// but may be false of the whole listing — the most misleading form
+ /// of truncation this path exists to prevent.
#[test]
fn an_id_miss_on_a_paginated_listing_says_the_row_may_be_later() {
let page = NextPage::of(&[param("start")], "https://api.mapbox.com/a?start=3");
@@ -2312,20 +2282,20 @@ mod tests {
assert_eq!(err.downcast_ref::().unwrap().fix, None);
}
- /// The shape mapbox/mcp-server fixed in `directions_tool`: a value spliced
- /// into the path used to append query parameters the caller never asked
- /// for. Verified against `reqwest::Url` at the time — `x?fresh=true` gave
- /// `query = fresh=true&access_token=…`.
+ /// The shape mapbox/mcp-server fixed in `directions_tool`: a spliced-in
+ /// value used to append query parameters the caller never asked for.
+ /// Verified against `reqwest::Url`: `x?extra=1` gave
+ /// `query = extra=1&access_token=…`.
#[test]
fn a_path_parameter_cannot_inject_a_query_string() {
- let safe = path_segment("style_id", "x?fresh=true").expect("encoded, not refused");
- assert_eq!(safe, "x%3Ffresh=true");
+ let safe = path_segment("style_id", "x?extra=1").expect("encoded, not refused");
+ assert_eq!(safe, "x%3Fextra=1");
}
- /// With the caller's token and the command's method attached, this aimed a
+ /// With the caller's token and method attached, this used to aim a
/// `DELETE` at whatever path the value resolved to. The host was never
- /// reachable — `//evil`, `https://evil` and `x@evil` all stay on
- /// `api.mapbox.com` — but the path was.
+ /// reachable this way (`//evil`, `https://evil`, `x@evil` all stay on
+ /// `api.mapbox.com`), but the path was.
#[test]
fn a_path_parameter_cannot_retarget_the_path() {
let safe = path_segment("style_id", "../../tokens/v2/victim").expect("encoded");
@@ -2333,18 +2303,18 @@ mod tests {
assert!(!safe.contains('/'), "one segment, not four: {safe}");
}
- /// `\` is a path separator too, for a special scheme — WHATWG says so and
- /// `reqwest::Url` agrees: `..\..\tokens` resolved just as `../../tokens`
- /// did. Encoding `/` alone would have left this open.
+ /// `\` is also a path separator for special schemes — WHATWG says so,
+ /// and `reqwest::Url` agrees: `..\..\tokens` resolves just like
+ /// `../../tokens`. Encoding `/` alone would leave this open.
#[test]
fn a_backslash_cannot_retarget_the_path_either() {
let safe = path_segment("style_id", r"..\..\tokens").expect("encoded");
assert_eq!(safe, "..%5C..%5Ctokens");
}
- /// A fragment is not sent to the server, so this silently truncated the
- /// path rather than redirecting it — a request to somewhere the caller
- /// could not see in what they typed.
+ /// A fragment is never sent to the server, so this used to silently
+ /// truncate the path instead of redirecting it — a request to
+ /// somewhere the caller couldn't see from what they typed.
#[test]
fn a_fragment_cannot_truncate_the_path() {
assert_eq!(
@@ -2353,11 +2323,11 @@ mod tests {
);
}
- /// **Refused, not encoded, and that distinction is the point.** Encoding
- /// does not stop a dot segment: WHATWG reads `%2e%2e` as one too, so a
- /// value of exactly `..` climbs a level however it is spelled. Encoding
- /// the separators handles the multi-level case by collapsing it into one
- /// segment; this handles what is left.
+ /// **Refused, not encoded — that distinction is the point.** Encoding
+ /// doesn't stop a dot segment: WHATWG reads `%2e%2e` as one too, so a
+ /// value of exactly `..` climbs a level no matter how it's spelled.
+ /// Encoding the separators handles the multi-level case (it collapses
+ /// into one segment); this handles what's left over.
#[test]
fn a_dot_segment_is_refused_however_it_is_spelled() {
for value in ["..", ".", "%2e%2e", "%2E%2E", "%2e", ".%2e", "%2e."] {
@@ -2366,19 +2336,19 @@ mod tests {
}
}
- /// Encoded dots *and* encoded slashes together, which is the shape that
- /// looks like traversal and is not.
+ /// Encoded dots *and* encoded slashes together — looks like traversal,
+ /// isn't.
///
- /// `%2f` is never decoded into a separator by the URL parser, so this
- /// stays one segment on the wire — unlike encoded dots with *raw* slashes
- /// (`%2e%2e/%2e%2e/x`), which the parser does resolve and which the
- /// encoding above is what stops. It is left alone on purpose: percent-
- /// encoded values are a documented Mapbox feature, not an attack
- /// signature. A custom marker overlay is
+ /// `%2f` is never decoded into a real separator by the URL parser, so
+ /// this stays one segment on the wire — unlike encoded dots with
+ /// *raw* slashes (`%2e%2e/%2e%2e/x`), which the parser does resolve,
+ /// and which the encoding above already stops. Left alone on purpose:
+ /// percent-encoded values are a documented Mapbox feature, not an
+ /// attack signature. A custom marker overlay is
/// `url-https%3A%2F%2Fexample.com%2Fmarker.png(…)`, `geojson(…)` takes
- /// URI-encoded GeoJSON, and the spec says a bbox's brackets "may be sent
- /// literally or percent-encoded as `%5B`". Refusing these would break all
- /// three.
+ /// URI-encoded GeoJSON, and the spec says a bbox's brackets "may be
+ /// sent literally or percent-encoded as `%5B`". Refusing these would
+ /// break all three.
#[test]
fn a_percent_encoded_separator_stays_one_segment() {
let value = "%2e%2e%2fvictim";
@@ -2399,10 +2369,10 @@ mod tests {
);
}
- /// **The reason this encodes four characters and not everything outside
- /// RFC 3986's unreserved set.** These values carry punctuation on purpose,
- /// and percent-encoding it would rewrite requests that work today —
- /// `static get-image`'s template alone is
+ /// **Why this encodes only four characters, not everything outside
+ /// RFC 3986's unreserved set.** These values carry punctuation on
+ /// purpose, and percent-encoding it would rewrite requests that work
+ /// today — `static get-image`'s template alone is
/// `…/static/{overlay}/{lon},{lat},{zoom},{bearing},{pitch}/{width}x{height}{highRes}{format}`.
#[test]
fn punctuation_a_path_parameter_legitimately_carries_is_untouched() {
@@ -2422,8 +2392,8 @@ mod tests {
}
}
- /// An empty optional parameter still drops its whole segment, which
- /// several operations rely on — encoding must not have taken that away.
+ /// Several operations rely on an empty optional parameter still
+ /// dropping its whole segment — encoding must not break that.
#[test]
fn an_empty_optional_parameter_still_drops_its_segment() {
let safe = path_segment("draft", "").expect("empty is not a dot segment");
diff --git a/src/spec.rs b/src/spec.rs
index cc797d4..8e4a413 100644
--- a/src/spec.rs
+++ b/src/spec.rs
@@ -29,10 +29,10 @@ pub struct Operation {
/// takes one at all.
pub body: Option,
pub base_url: String,
- /// Set when this operation's spec-documented required scope does not exist
- /// as a registrable OAuth scope (see UNSUPPORTED_OPERATIONS below). A token
- /// obtained via `mapbox auth login` can never carry it, so the command refuses to
- /// run instead of failing with a confusing 403 at request time.
+ /// Set when the operation needs a scope that isn't registrable as OAuth
+ /// (see `UNSUPPORTED_OPERATIONS` below). A `mapbox auth login` token can
+ /// never carry that scope, so the command refuses to run instead of
+ /// failing later with a confusing 403.
pub disabled_scope: Option<&'static str>,
/// The operation that shows one of the things this one lists, when the
/// spec describes such a pair. Filled in by [`link_detail_operations`].
@@ -50,97 +50,93 @@ pub struct Operation {
pub deprecated: bool,
/// Set for an operation listed in [`WITHHELD_OPERATIONS`].
withheld: bool,
- /// Extra, visible names this command answers to and lists in `--help`
- /// and `--schema` — see [`COMMAND_ALIASES`]. Empty for every operation
- /// but the one whose spec-generated name is worth keeping alongside a
+ /// Extra, visible names this command answers to — listed in `--help`
+ /// and `--schema`. See [`COMMAND_ALIASES`]. Empty except for the one
+ /// operation whose spec-generated name is worth keeping alongside a
/// better one.
pub aliases: Vec<&'static str>,
- /// Extra names this command answers to but does not offer anyone — also
- /// from [`COMMAND_ALIASES`]. A caller already using one keeps working,
- /// and nothing publishes it: not `--help`, not `--schema`, not the docs
- /// page. Only `clap` and this field know it is there, which is why
- /// `api_command_surface`'s fixture reads the built command tree rather
- /// than the schema — the schema cannot see one by design.
+ /// Extra names this command answers to but doesn't advertise anywhere
+ /// — not `--help`, not `--schema`, not the docs page. Also from
+ /// [`COMMAND_ALIASES`]. A caller already using one keeps working. Only
+ /// `clap` and this field know it exists, which is why
+ /// `api_command_surface`'s fixture reads the built command tree instead
+ /// of the schema, which can't see hidden aliases by design.
///
- /// `String`, not `&'static str`: what lands here is the *generated*
- /// name (`camel_to_kebab` of the spec's own `operationId`), computed at
+ /// `String`, not `&'static str`, because this holds the *generated*
+ /// name (`camel_to_kebab` of the spec's `operationId`), computed at
/// parse time rather than written in the table.
pub hidden_aliases: Vec,
}
-/// (service, operationId, the media type the API actually requires) for
-/// bodies whose spec is wrong.
+/// (service, operationId, the media type the API actually wants) for
+/// operations whose spec gets the content type wrong.
///
-/// Not a workaround for a hard case — a correction. `starFile` declares
-/// `application/json` with a `boolean` schema, and the service answers
-/// `400 Must be plaintext true or false` to exactly that. Sending the same
-/// `true` as `text/plain` returns 204. Verified with curl against
-/// production: `application/json` and no content type both fail, `text/plain`
-/// succeeds.
+/// `starFile`'s spec says `application/json` with a `boolean` schema, but
+/// the service rejects that with `400 Must be plaintext true or false`.
+/// Sending the same `true` as `text/plain` works (204). Verified with curl
+/// against production.
///
-/// Kept as data rather than a branch in the executor so the divergence is
-/// visible next to the operation it belongs to, and so a spec fix is a
-/// deletion from this list.
+/// Kept as a table instead of a branch in the executor, so the fix is
+/// visible next to the operation it's for, and removing it later is just
+/// deleting a row.
const BODY_CONTENT_TYPE_OVERRIDES: &[(&str, &str, &str)] = &[("styles", "starFile", "text/plain")];
/// The media types an operation's request body may be sent as.
///
-/// This used to be a bare `has_body: bool`, which threw the declared type
-/// away and left the executor with one choice: call everything
-/// `application/json`. That is right for the twenty JSON operations and
-/// wrong for the three that are not — `uploadSpriteImage` wants raw SVG
-/// bytes, `batchUploadSprite` a multipart form, `uploadChunk` raw bytes —
-/// so those had no way to send a body at all.
+/// This used to be a plain `has_body: bool`, which forced the executor to
+/// always send `application/json`. That's right for most operations but
+/// wrong for three: `uploadSpriteImage` (raw SVG bytes), `batchUploadSprite`
+/// (multipart form), and `uploadChunk` (raw bytes). Those three had no way
+/// to send a body at all under the old design.
#[derive(Debug, Clone)]
pub struct RequestBody {
- /// Whether the operation refuses to work without one.
+ /// Whether the operation refuses to work without a body.
///
- /// Read but not enforced: clap still accepts a bodyless invocation of
- /// `create-style`, and turning that into a local error is a change to
- /// what the CLI rejects rather than to what it describes. It is here so
- /// `--schema` can say which bodies the API insists on — thirteen of the
- /// sixteen body-carrying operations the CLI exposes, which between them
- /// declare eighteen flags — instead of calling every one of them
- /// optional.
+ /// We read this but don't enforce it: clap still lets you run
+ /// `create-style` with no body. Rejecting that locally would change what
+ /// the CLI refuses, not just what it describes. This field exists so
+ /// `--schema` can correctly say which bodies the API requires, instead
+ /// of calling all of them optional.
pub required: bool,
/// Declared media types, in the order the spec lists them. Usually one;
/// `initUpload` declares both `application/octet-stream` and
/// `application/json`.
pub content_types: Vec,
- /// The multipart property the files go under — `images` for
- /// `batchUploadSprite`. Read from the schema rather than hard-coded:
- /// it is the field name the API matches on, so guessing it wrong fails
- /// at request time with nothing to point at.
+ /// The multipart field the files go under — `images` for
+ /// `batchUploadSprite`. Read from the schema instead of hard-coded,
+ /// since the API matches on this field name exactly; guessing it wrong
+ /// would fail the request with no clear error.
pub multipart_field: Option,
}
pub const MULTIPART: &str = "multipart/form-data";
-/// The path placeholders the global `--username` fills, rather than a
-/// parameter of their own.
+/// The path placeholders that the global `--username` flag fills, rather
+/// than each becoming its own parameter.
///
-/// One list, because three places act on it and they have to agree:
-/// `parse_spec` drops the matching parameters, `executor::execute`
-/// substitutes them, `link_detail_operations` refuses to treat them as an
-/// identifier, and `crate::schema` describes the flag that fills them. A
-/// fourth spelling appearing in a spec and being added to only some of those
-/// is the failure this prevents; `every_url_placeholder_has_an_argument`
-/// fails the build if the schema and this list ever come apart.
+/// Kept as one shared list because four places need to agree on it:
+/// `parse_spec` drops these as parameters, `executor::execute` fills them
+/// in, `link_detail_operations` won't treat them as an identifier, and
+/// `crate::schema` describes the flag that fills them. If a spec used a new
+/// spelling and only some of those four places knew about it, things would
+/// break inconsistently. `every_url_placeholder_has_an_argument` fails the
+/// build if this list and the schema ever disagree.
pub const ACCOUNT_PLACEHOLDERS: [&str; 3] = ["username", "owner", "account"];
impl RequestBody {
/// Whether `--data` applies.
///
/// An empty list means the spec declared a `requestBody` without saying
- /// what goes in it. The CLI has always assumed JSON there, and still
- /// does — the alternative is withdrawing a flag that works today.
+ /// what type it is. The CLI has always assumed JSON in that case, and
+ /// still does — the alternative would be removing a flag that works
+ /// today.
pub fn accepts_json(&self) -> bool {
self.content_types.is_empty() || self.content_types.iter().any(|ct| is_json(ct))
}
- /// The media type a `--file` would be sent as: the first declared type
- /// that is not JSON. `None` when the body is JSON-only, which is what
- /// makes `--file` a flag those commands never show.
+ /// The media type `--file` would send: the first declared type that
+ /// isn't JSON. `None` for a JSON-only body — that's why `--file` never
+ /// shows up on those commands.
pub fn file_content_type(&self) -> Option<&str> {
self.content_types
.iter()
@@ -154,10 +150,10 @@ impl RequestBody {
.is_some_and(|ct| essence(ct) == MULTIPART)
}
- /// The media type for a body that is text the caller types, not a file.
+ /// The media type for a body that's typed text, not a file.
///
- /// `--data` carries it, unparsed: `starFile`'s whole body is the word
- /// `true`, and there is nothing to be gained by making that a file.
+ /// `--data` carries it as-is. `starFile`'s whole body is just the word
+ /// `true`, so there's no point making that a file.
pub fn text_content_type(&self) -> Option<&str> {
self.content_types
.iter()
@@ -191,51 +187,50 @@ fn is_json(content_type: &str) -> bool {
essence == "application/json" || essence.ends_with("+json")
}
-/// (service name, operationId, why) for operations this CLI declines to
+/// (service name, operationId, why) for operations this CLI chooses not to
/// expose.
///
-/// Distinct from `UNSUPPORTED_OPERATIONS`, which lists what the platform
-/// makes impossible. These would work; we choose not to offer them, so the
-/// reason has to be written down or a later reader will "fix" the omission.
+/// Different from `UNSUPPORTED_OPERATIONS`, which lists what the platform
+/// makes impossible. These operations would work fine — we just chose not
+/// to offer them — so the reason has to be written down, or a later reader
+/// might "fix" the omission by mistake.
const WITHHELD_OPERATIONS: &[(&str, &str, &str)] = &[
// "Lock or unlock a style from editing and deletion." Unlocking is the
- // dangerous half: it turns a protected style into a deletable one, and a
- // CLI makes that a single line with no confirmation.
+ // dangerous half: it turns a protected style into a deletable one, and
+ // a CLI would make that a single line with no confirmation.
//
// Verified 2026-09-08 against production with a real token holding
- // `styles:protect` (already registrable): the endpoint
- // works — `PUT .../protected` returned 200. It rejects a JSON body
+ // `styles:protect` (already registrable): the endpoint works —
+ // `PUT .../protected` returns 200. It rejects a JSON body
// (`{"protected":false}` → 400 "Must be plaintext true or false") and
- // wants the literal string `true`/`false` instead. So this is not
- // platform-blocked the way the admin-only style endpoints below are —
- // it is withheld purely for the safety reason above, not for lack of
- // access.
+ // wants the literal string `true`/`false` instead. So unlike the
+ // admin-only endpoints below, this isn't blocked by the platform — we're
+ // withholding it purely for the safety reason above.
(
"styles",
"setStyleProtected",
"unlocks a style for deletion",
),
- // Admin-only, and gated on a role rather than a scope: a token from
- // `mapbox auth login` gets a bare 403 with no scope named, including for
- // an account that belongs to Mapbox.
+ // Admin-only, gated by role rather than scope. A `mapbox auth login`
+ // token gets a bare 403 with no scope named, even for a Mapbox-owned
+ // account.
("styles", "adminGetStyle", "admin-only endpoint"),
("styles", "adminUpdateStyle", "admin-only endpoint"),
- // 3D model assets are a different product surface from the glyph and
- // metadata endpoints the rest of `fonts` covers, and nothing here has
- // ever fetched one — no account reachable from this CLI has a model to
- // ask for, so the command would ship untested.
+ // 3D model assets are a different product from the glyph/metadata
+ // endpoints the rest of `fonts` covers, and we've never fetched one —
+ // no account we can reach has a model to test against, so shipping this
+ // command would mean shipping it untested.
//
- // Verified 2026-09-08: this is purely a test-data gap, not a scope
- // problem. A request for a nonexistent model against production returned
- // 404 "Model ... not found", not 403 — confirming the endpoint only
- // needs `fonts:read`, which `DEFAULT_SCOPES` already requests.
+ // Verified 2026-09-08: this is a test-data gap, not a scope problem. A
+ // request for a nonexistent model returned 404 "Model ... not found",
+ // not 403 — so the endpoint only needs `fonts:read`, which
+ // `DEFAULT_SCOPES` already requests.
("fonts", "getModelAsset", "not supported yet"),
- // The v1-v3 API is dead to this CLI. `getLegacyTile` answers 410 for
- // every request, which the spec agrees is correct — it documents the
- // endpoint as deprecated and no longer supported. `getLegacyGrid`
- // rejects a token from `mapbox auth login` with a JSONP-wrapped 401,
- // for every version and every tileset, including the one whose grid
- // `getGrid` returns. Neither can succeed, so neither is a command.
+ // The v1-v3 API is dead to this CLI. `getLegacyTile` answers 410 to
+ // every request — the spec agrees, marking it deprecated and no longer
+ // supported. `getLegacyGrid` rejects a `mapbox auth login` token with a
+ // JSONP-wrapped 401, for every version and tileset, including the one
+ // `getGrid` works on. Neither can succeed, so neither is a command.
(
"maps",
"getLegacyGrid",
@@ -250,64 +245,59 @@ fn withheld(service_name: &str, operation_id: &str) -> bool {
.any(|(svc, op, _)| *svc == service_name && *op == operation_id)
}
-/// Some commands get an ugly name because we build `command_name` from the
-/// spec's own `operationId`, and once in a while that `operationId` is bad
-/// — tilequery's names its one operation after its own URL path instead of
-/// after what it does. We can't fix the spec, so this table gives that
-/// command a better name to go by instead.
+/// We build `command_name` from the spec's own `operationId`, and sometimes
+/// that gives a command an ugly name — tilequery names its one operation
+/// after its URL path instead of what it does. We can't fix the spec, so
+/// this table gives that command a better name instead.
///
/// Table shape: `(service, operationId, alias, show_generated_name)`.
-/// - `service` / `operationId`: which operation this is about — same key
-/// `WITHHELD_OPERATIONS` and `BODY_CONTENT_TYPE_OVERRIDES` use.
+/// - `service` / `operationId`: which operation this is about — the same
+/// key `WITHHELD_OPERATIONS` and `BODY_CONTENT_TYPE_OVERRIDES` use.
/// - `alias`: the better name.
-/// - `show_generated_name`: `true` keeps the generated name as the command
-/// and makes the alias a second, equally visible way to write it.
-/// `false` swaps them — the alias becomes the command, and the generated
-/// name survives only as a hidden alias: it still runs, and nothing
-/// names it anywhere, `--help`, `--schema` and `docs/commands.md`
-/// alike.
+/// - `show_generated_name`: `true` keeps the generated name as the
+/// command and adds the alias as a second, equally visible spelling.
+/// `false` swaps them: the alias becomes the command, and the
+/// generated name survives only as a hidden alias — it still runs, but
+/// nothing publishes it (not `--help`, not `--schema`, not
+/// `docs/commands.md`).
///
-/// Empty today. Its one row renamed tilequery's operation to `get-tilequery`,
-/// and [`CLI_COMMAND_EXTENSION`] now names that operation `tilequery get`
-/// directly — a renaming table beside a renaming extension is two answers to
-/// one question. The mechanism stays because the extension cannot express
-/// the other half of what a row does: keep a retired spelling running while
-/// publishing it nowhere.
+/// Empty today. Its one row used to rename tilequery's operation to
+/// `get-tilequery`; [`CLI_COMMAND_EXTENSION`] now names that operation
+/// `tilequery get` directly, making the row redundant. The mechanism stays
+/// because the extension can't do the other thing a row can: keep an old
+/// spelling working while publishing it nowhere.
const COMMAND_ALIASES: &[(&str, &str, &str, bool)] = &[];
/// The OpenAPI extension that says where an operation's command belongs:
/// `x-mapbox-cli-command: [service, path_segment...]`.
///
-/// A maintainer-only step writes it onto every operation it keeps, from a
-/// per-operation decision record for what belongs in this CLI's command
-/// surface. So `openapi/` carries the whole answer and nothing here has a
-/// second opinion about it.
-/// Two consequences nothing else in this file would lead you to expect:
+/// This is written onto every operation `openapi/` keeps, so `openapi/`
+/// carries the full answer and nothing here second-guesses it. Two things
+/// to know:
///
-/// - **The first element is the real service, and it need not be the one
-/// the spec file is wired under.** `styles.yaml`'s sprite operations name
+/// - **The first element is the real service, and doesn't have to match
+/// the spec file it came from.** `styles.yaml`'s sprite operations name
/// `sprites`; the one operation in `vectortiles.yaml` names `tilesets`.
-/// A service in [`MAPBOX_SPEC_ENTRIES`] can therefore end up with no
-/// operations at all, and a service nothing wires can end up with five.
-/// [`regroup_by_service`] is where that happens, and it is the reason
-/// `parse_spec`'s answer is an intermediate rather than the surface.
-/// - **The rest is a path, not a name.** More than one segment nests:
-/// `["draft", "get"]` puts the command under an intermediate `draft`
-/// group, as `mapbox styles draft get`.
+/// So a service in [`MAPBOX_SPEC_ENTRIES`] can end up with zero
+/// operations, while a service no file is wired under can end up with
+/// five. [`regroup_by_service`] does that regrouping — which is why
+/// `parse_spec`'s output is an intermediate step, not the final surface.
+/// - **The rest is a path, not a single name.** More than one segment
+/// nests: `["draft", "get"]` puts the command under a `draft` group, as
+/// `mapbox styles draft get`.
///
-/// Absent from everything in `custom-openapi/`, which the decision record
-/// does not cover. Those keep the pre-extension behavior: their file's own
-/// service name, and one flat generated command name.
+/// Not present in `custom-openapi/` files — those keep the older behavior:
+/// their file's own service name, and one flat generated command name.
const CLI_COMMAND_EXTENSION: &str = "x-mapbox-cli-command";
/// Where [`CLI_COMMAND_EXTENSION`] says this operation's command goes, as
-/// (service, command path). `None` for an operation that declares none.
+/// (service, command path). `None` if the operation declares nothing.
///
-/// A declared-but-unusable value is an error rather than a fallback. It can
-/// only come from a bug in the strip step, and the quiet reading of it —
-/// keeping the file's own service and the generated name — is a command that
-/// silently appears under the wrong service, which is exactly the failure
-/// this extension exists to prevent.
+/// A value that's present but malformed is an error, not something to fall
+/// back from. It can only mean a bug in the strip step, and silently
+/// falling back to the file's own service and generated name would put the
+/// command under the wrong service — exactly what this extension exists to
+/// prevent.
fn cli_command_target(
op: &Value,
operation_id: Option<&str>,
@@ -341,12 +331,13 @@ fn cli_command_target(
}
/// The alias for this operation, if it has one, and whether the generated
-/// name it stands in for should keep showing up alongside it.
+/// name it replaces should still show up alongside it.
///
-/// A row whose `service` or `operationId` matches nothing answers `None` for
-/// every operation, and a dead row looks exactly like a working one from
-/// here — so `every_command_alias_names_a_real_operation` checks the other
-/// end, that each row actually reaches an operation.
+/// A row whose `service` or `operationId` doesn't match anything just
+/// returns `None` here, same as a working row that hasn't matched yet — so
+/// a typo'd row would go unnoticed from this function alone.
+/// `every_command_alias_names_a_real_operation` is the test that checks
+/// each row actually reaches a real operation.
fn alias_for(service_name: &str, operation_id: &str) -> Option<(&'static str, bool)> {
COMMAND_ALIASES
.iter()
@@ -354,22 +345,24 @@ fn alias_for(service_name: &str, operation_id: &str) -> Option<(&'static str, bo
.map(|(_, _, alias, show_generated)| (*alias, *show_generated))
}
-/// What an operation ends up called, given the name its `operationId`
-/// generated and whatever [`COMMAND_ALIASES`] says about it: the command
-/// name, its visible aliases, and its hidden ones.
+/// What an operation ends up called, combining its generated name with
+/// whatever [`COMMAND_ALIASES`] says about it: the command name, its
+/// visible aliases, and its hidden ones.
///
-/// Split out of `parse_spec` so both answers can be tested without a row in
-/// the real table standing for them — today every row says `false`, which
-/// would leave the `true` arm running nowhere.
+/// Split out of `parse_spec` so both branches below can be tested directly,
+/// without needing a real row in the table for each — every row in the
+/// table today says `false`, so the `true` branch would otherwise never
+/// run.
fn command_names(
generated_name: String,
alias: Option<(&'static str, bool)>,
) -> (String, Vec<&'static str>, Vec) {
match alias {
// The generated name stays the command; the alias is a second
- // spelling `--help` lists beside it.
+ // spelling that `--help` also lists.
Some((alias, true)) => (generated_name, vec![alias], vec![]),
- // The alias *is* the command, and the generated name goes quiet.
+ // The alias becomes the command; the generated name still works
+ // but is hidden.
Some((alias, false)) => (alias.to_string(), vec![], vec![generated_name]),
None => (generated_name, vec![], vec![]),
}
@@ -390,17 +383,17 @@ impl Operation {
format!("{} {}", self.service, self.command_path.join(" "))
}
- /// Whether this is a service's own liveness probe rather than something
- /// a person or an agent would ask for.
+ /// Whether this is a service's own health check, rather than something
+ /// a person or an agent would actually want to call.
///
- /// Every spec that has one puts it at the service root or at a
- /// conventional health-check path, and matching on the path catches all
- /// five — two of them have no `operationId` at all, so their command
- /// names are generated from a summary and cannot be matched by name.
+ /// Every spec that has one puts it at the service root or a
+ /// conventional health-check path, so matching on the path catches all
+ /// five of them. Two have no `operationId` at all, so their command
+ /// names come from a summary and can't be matched by name.
///
- /// Excluded for a different reason from `UNSUPPORTED_OPERATIONS`: these
- /// are not impossible, they are simply not this CLI's business. Three of
- /// the five answer 404 in production anyway.
+ /// Excluded for a different reason than `UNSUPPORTED_OPERATIONS`: these
+ /// aren't impossible to call, they're just not this CLI's business.
+ /// Three of the five return 404 in production anyway.
pub fn is_liveness_probe(&self) -> bool {
matches!(self.path_template.as_str(), "/" | "/mbx-health")
}
@@ -413,11 +406,11 @@ impl Operation {
/// Whether the operation is part of the command surface at all.
///
- /// One predicate rather than three repeated conditions, because two
- /// places have to agree on it exactly: the command tree
+ /// One shared check instead of three repeated conditions, because two
+ /// places must agree exactly: the command tree
/// (`build_service_command`) and the schema (`crate::schema`). An
- /// operation described but not runnable, or runnable but not described,
- /// is worse than one that is neither.
+ /// operation that's described but not runnable — or runnable but not
+ /// described — is worse than one that's neither.
pub fn is_exposed(&self) -> bool {
self.disabled_scope.is_none() && !self.is_liveness_probe() && !self.is_withheld()
}
@@ -425,14 +418,14 @@ impl Operation {
/// Whether running this operation changes something on Mapbox's side —
/// which is what earns it a `--dry-run`.
///
- /// The HTTP method is the answer, and deliberately the only one. It is
- /// the single signal every spec carries, so the rule stays correct for a
- /// service nobody has looked at and for the next one that lands in
- /// `openapi-specs`; a hand-kept list of mutating operation IDs would go
- /// stale on the first sync. The cost is that a POST which only reads —
- /// `batchGeocode` posts a query and gets answers back — is offered a
- /// `--dry-run` it does not need. That is a spare flag on a handful of
- /// commands, against the alternative of a missing one on a `delete`.
+ /// We use the HTTP method, and only the HTTP method, deliberately. It's
+ /// the one signal every spec carries, so the rule stays correct even
+ /// for a service nobody's looked at yet, or the next one added to
+ /// `openapi-specs`. A hand-kept list of mutating operation IDs would go
+ /// stale on the first sync. The cost: a read-only POST like
+ /// `batchGeocode` (posts a query, gets answers back) gets an unneeded
+ /// `--dry-run`. That's a harmless spare flag on a few commands, versus
+ /// the alternative of a missing one on a real `delete`.
pub fn is_mutating(&self) -> bool {
matches!(
self.method.to_ascii_uppercase().as_str(),
@@ -444,16 +437,15 @@ impl Operation {
/// A sibling operation that lists the items another one shows one of.
#[derive(Debug, Clone)]
pub struct ListingOperation {
- /// The whole command minus `mapbox`: `styles list`. Service included,
- /// because a pair can straddle two services now — the operations are
- /// paired by URL path, and `x-mapbox-cli-command` is free to file the two
- /// ends of one path under different services.
+ /// The whole command minus `mapbox`: `styles list`. Includes the
+ /// service, because a pair can straddle two services — operations are
+ /// paired by URL path, and `x-mapbox-cli-command` can file either end
+ /// of one path under a different service.
pub command: String,
- /// The listing's own path. It is the detail operation's path less its
- /// last segment, but carrying it means a caller can see which of the
- /// failing operation's path parameters the listing needs as well —
- /// `/styles/v1/{username}/{style_id}/sprite` still wants a style — and
- /// does not have to re-derive an invariant this pass already knows.
+ /// The listing's own path — the detail operation's path minus its last
+ /// segment. Kept here so a caller can see which path parameters the
+ /// listing itself needs too (`/styles/v1/{username}/{style_id}/sprite`
+ /// still needs a style), instead of re-deriving that elsewhere.
pub path_template: String,
}
@@ -466,51 +458,50 @@ pub struct DetailOperation {
pub parameter: String,
}
-/// (service name, operationId, scope) for operations whose spec-documented required
-/// scope is absent from the Accounts API's registration allowlist,
-/// audited 2026-08-28. Re-check before removing an entry — these can't be
-/// fixed from this side: the fix is Mapbox making the scope registrable, and
-/// an entry comes off this list once a direct `POST /oauth/register` grants
-/// it back unchanged.
+/// (service name, operationId, scope) for operations whose required scope
+/// isn't in the Accounts API's registration allowlist. Audited 2026-08-28.
///
-/// `fonts:list` and `fonts:write` (and therefore `listFonts`, `uploadFont`,
-/// `deleteFont` and `updateFontMetadata`) came off this list once both
-/// became registrable on 2026-09-08. `DEFAULT_SCOPES` in `auth.rs` requests
-/// them now, so all four commands ship. `updateFontMetadata` briefly sat in
-/// `WITHHELD_OPERATIONS` instead: a same-day retest with a `mapbox auth
-/// login` token found `PATCH .../{face}/metadata` 404 "Font not found for
-/// expected owner" against a font the same token could upload, read and
-/// delete at that exact path. A later retest, same day, with a freshly
-/// uploaded font and the same token, got 200 both ways (`visibility`
-/// flipped and stuck, confirmed with `get-font-metadata --fresh`) — most
-/// likely the scope's authorization hadn't finished propagating yet right
-/// after the scopes became registrable. Moved back here rather than back to
-/// `WITHHELD_OPERATIONS`, since the 404 was a propagation delay, not
-/// something about the `tk` usage code.
+/// Re-check before removing an entry. We can't fix these ourselves — Mapbox
+/// has to make the scope registrable — and an entry comes off once a direct
+/// `POST /oauth/register` grants it back unchanged.
+///
+/// `fonts:list` and `fonts:write` came off this list on 2026-09-08 once
+/// both became registrable, which is why `listFonts`, `uploadFont`,
+/// `deleteFont` and `updateFontMetadata` all ship today (`DEFAULT_SCOPES`
+/// in `auth.rs` now requests both scopes). `updateFontMetadata` briefly sat
+/// in `WITHHELD_OPERATIONS` instead: a same-day retest found
+/// `PATCH .../{face}/metadata` returning 404 "Font not found for expected
+/// owner" against a font the same token could otherwise upload, read and
+/// delete. A later retest that same day, on a freshly uploaded font, got
+/// 200 both ways — most likely the new scope's authorization just hadn't
+/// finished propagating yet. Moved back here rather than left in
+/// `WITHHELD_OPERATIONS`, since the 404 was a propagation delay, not a real
+/// problem with the operation.
const UNSUPPORTED_OPERATIONS: &[(&str, &str, &str)] = &[
- // Confirmed live 2026-09-08: `fonts:metadata` is a real scope name, not a
- // documentation typo — the endpoint answers 403 "This API requires a
- // token with fonts:metadata scope" verbatim. It just isn't in
- // the registration allowlist yet.
+ // Confirmed live 2026-09-08: `fonts:metadata` is a real scope name, not
+ // a typo in the docs — the endpoint literally answers 403 "This API
+ // requires a token with fonts:metadata scope". It's just not in the
+ // registration allowlist yet.
("fonts", "getFontCoverage", "fonts:metadata"),
- // tokens:write is not registrable either — confirmed by a direct
- // POST /oauth/register against production: it's silently dropped from the
- // granted scope even when requested via both body and query. It only exists
- // in the classic, role-gated (ADMIN-only) token-creation path, not DCR.
+ // tokens:write isn't registrable either — confirmed with a direct
+ // POST /oauth/register against production, which silently drops it
+ // from the granted scope even when requested via both body and query.
+ // It only exists in the older, role-gated (ADMIN-only) token-creation
+ // path, not this newer one (DCR).
("accounts", "createToken", "tokens:write"),
("accounts", "updateToken", "tokens:write"),
("accounts", "deleteToken", "tokens:write"),
- // Found by running it, not by reading: the styles spec documents no
- // scope, and an early probe answered 403 "requires a token with
- // styles:download scope". `POST /oauth/register` then drops
- // `styles:download` from the granted set, so no login can carry it.
+ // Found by testing it, not by reading the spec: the styles spec
+ // documents no scope, but an early probe got 403 "requires a token
+ // with styles:download scope". `POST /oauth/register` then drops
+ // `styles:download` from the granted set, so no login can get it.
//
- // Deeper still (verified 2026-09-08 with a real token, no scope
+ // Deeper problem too (verified 2026-09-08 with a real token, no scope
// involved): the endpoint now answers 403 "This is a prerelease API.
- // Please contact support at help@mapbox.com to request access." —
- // access is gated per-account, not by OAuth scope at all. Making
- // `styles:download` registrable would not unblock this command by
- // itself; that's why it wasn't made registrable alongside the other two
+ // Please contact support at help@mapbox.com to request access." So
+ // access is gated per-account, not by OAuth scope at all — making
+ // `styles:download` registrable wouldn't unblock this command by
+ // itself, which is why it wasn't registered alongside the other two
// fonts scopes.
("styles", "downloadStyleZip", "styles:download"),
];
@@ -530,14 +521,14 @@ pub struct Parameter {
pub description: Option,
pub enum_values: Vec,
pub is_boolean: bool,
- /// The spec's `type`, when it is one clap can check before we spend a
- /// request finding out. `boolean` is absent here because it is handled
- /// as a flag rather than a value.
+ /// The spec's `type`, when clap can check it before we spend a request
+ /// finding out the value is wrong. No `boolean` here — that's handled
+ /// as a flag instead of a value.
pub numeric: Option,
}
-/// A numeric parameter's width, kept apart from the generic string case so
-/// the value can still be handed to the query string as text.
+/// A numeric parameter's width. Kept separate from the plain string case,
+/// but the value is still sent to the query string as text either way.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Numeric {
Integer,
@@ -547,34 +538,26 @@ pub enum Numeric {
/// One spec file's embedded OpenAPI YAML, with the name this repo files it
/// under, ready for [`parse_spec`].
///
-/// `name` is no longer necessarily a service anyone can type.
-/// [`CLI_COMMAND_EXTENSION`] decides that per operation, so this is the
-/// file's own name: the fallback service for an operation that declares no
-/// target, the key `WITHHELD_OPERATIONS` and its neighbors match on, and
-/// what a maintainer-only drift check compares the two tables by. `maps` is the
-/// clearest case — the entry is still called that, and the service it used
-/// to produce is gone.
+/// `name` isn't necessarily a service anyone can type — that's decided per
+/// operation by [`CLI_COMMAND_EXTENSION`] instead. This is just the file's
+/// own name: the fallback service for an operation that declares no
+/// target, the key that `WITHHELD_OPERATIONS` and similar tables match on,
+/// and what a drift check compares those tables against. `maps` is the
+/// clearest example — the entry is still called that, even though the
+/// service it used to produce is gone.
#[derive(Clone, Copy)]
pub struct SpecEntry {
pub name: &'static str,
pub yaml: &'static str,
}
-/// Every spec file that comes from openapi-specs. Each `yaml` is pulled in
-/// via `include_str!` from `openapi/`, a vendored copy this repo owns.
+/// Every spec file this CLI ships with. Each `yaml` is pulled in via
+/// `include_str!` from `openapi/`, a copy this repo vendors.
///
/// Not what the CLI builds commands from — see [`effective_services`], and
/// [`CLI_COMMAND_EXTENSION`] for why a file's name and a service's name are
/// two different things.
///
-/// `openapi/` no longer mirrors upstream verbatim: every operation a
-/// maintainer-only decision record doesn't mark `enabled` is stripped out
-/// before this file ever sees it. `sources` isn't listed below for that
-/// reason: every operation in it is disabled there — it is not exposed as
-/// a command, and this repo drops it from the running CLI now rather than
-/// carrying a command group with nothing in it — so the vendoring step no
-/// longer writes a file for it at all.
-///
/// `maps` and `vectortiles` are listed, but each contributes only one
/// operation, and both now target the merged `tilesets` command group
/// (#116) rather than a group of their own — see [`MERGED_SERVICES`].
@@ -595,10 +578,6 @@ pub const MAPBOX_SPEC_ENTRIES: &[SpecEntry] = &[
name: "maps",
yaml: include_str!("../openapi/api-rastertiles/rastertiles.production.v1.yaml"),
},
- SpecEntry {
- name: "rasterarrays",
- yaml: include_str!("../openapi/api-rasterarrays/rasterarrays.production.v1.yaml"),
- },
SpecEntry {
name: "static-images",
yaml: include_str!("../openapi/api-gl/static-images.production.v1.yaml"),
@@ -622,30 +601,30 @@ pub const MAPBOX_SPEC_ENTRIES: &[SpecEntry] = &[
];
/// Services whose spec this repo writes and versions itself, under
-/// `custom-openapi//openapi/.yaml` — one level up, not two:
-/// `include_str!("../custom-openapi/search/openapi/search.yaml")`.
+/// `custom-openapi//openapi/.yaml` (one level up, not two —
+/// see `include_str!("../custom-openapi/search/openapi/search.yaml")`).
///
-/// For an API openapi-specs doesn't publish a usable spec for yet. A name
-/// here wins over the same name in [`MAPBOX_SPEC_ENTRIES`]; delete the
-/// override once upstream ships the service — a maintainer-only drift check
-/// flags a name wired on both sides for exactly that reason.
+/// For an API that `openapi-specs` doesn't publish a usable spec for yet. A
+/// name here wins over the same name in [`MAPBOX_SPEC_ENTRIES`]. Delete the
+/// override once upstream ships the service — a drift check flags a name
+/// wired on both sides, for exactly this reason.
pub const CUSTOM_SPEC_ENTRIES: &[SpecEntry] = &[SpecEntry {
name: "search",
yaml: include_str!("../custom-openapi/search/openapi/search.yaml"),
}];
-/// The list the CLI actually generates commands from: [`MAPBOX_SPEC_ENTRIES`]
-/// with each [`CUSTOM_SPEC_ENTRIES`] override swapped in, then the
-/// custom-only services appended.
+/// The list the CLI actually generates commands from: [`MAPBOX_SPEC_ENTRIES`],
+/// with each [`CUSTOM_SPEC_ENTRIES`] override swapped in and the
+/// custom-only services added at the end.
pub fn effective_spec_entries() -> Vec {
merge_entries(MAPBOX_SPEC_ENTRIES, CUSTOM_SPEC_ENTRIES)
}
/// The services the CLI builds its command tree from: every wired spec
-/// parsed, then re-bucketed by each operation's own target service.
+/// parsed, then regrouped by each operation's own target service.
///
-/// One function because the two steps are not separable — see
-/// [`regroup_by_service`] — and every caller wants the pair.
+/// One function because the two steps can't be separated — see
+/// [`regroup_by_service`] — and every caller needs both anyway.
pub fn effective_services() -> Result> {
let parsed: Vec = effective_spec_entries()
.iter()
@@ -654,9 +633,10 @@ pub fn effective_services() -> Result> {
Ok(regroup_by_service(parsed))
}
-/// Split out so the precedence rule is testable against small tables.
-/// Custom wins by replacement, in place — appending would leave two entries
-/// claiming one service name, which clap only refuses in a debug build.
+/// Split out so this precedence rule can be tested against small tables.
+/// Custom wins by replacing the entry in place — appending instead would
+/// leave two entries claiming one service name, which clap only catches in
+/// a debug build.
fn merge_entries(mapbox: &[SpecEntry], custom: &[SpecEntry]) -> Vec {
assert_no_duplicate_name(mapbox, "MAPBOX_SPEC_ENTRIES");
assert_no_duplicate_name(custom, "CUSTOM_SPEC_ENTRIES");
@@ -686,10 +666,10 @@ fn merge_entries(mapbox: &[SpecEntry], custom: &[SpecEntry]) -> Vec {
merged
}
-/// A second entry for the same name would otherwise vanish silently — the
-/// second one dropped by [`merge_entries`]'s `find`/filter with no trace of
-/// which spec was discarded. Catches it in either table, before the merge
-/// hides it.
+/// Without this, a second entry with the same name would silently
+/// disappear — [`merge_entries`]'s `find`/filter just drops it, with no
+/// record of which spec got discarded. This catches the duplicate in
+/// either table before the merge can hide it.
fn assert_no_duplicate_name(entries: &[SpecEntry], table: &str) {
let mut seen = std::collections::HashSet::new();
for entry in entries {
@@ -701,16 +681,16 @@ fn assert_no_duplicate_name(entries: &[SpecEntry], table: &str) {
}
}
-/// Title and description for a service no spec file is wired under.
+/// Title and description for a service that no spec file is wired under.
///
-/// [`CLI_COMMAND_EXTENSION`] assembles these out of operations that live in
-/// other files — `sprites` out of five of `styles.yaml`'s, `tilesets` out of
-/// one each from `rastertiles.yaml`, `rasterarrays.yaml`, `tilequery.yaml`
-/// and `vectortiles.yaml` (#116), `static` out of one each from
-/// `static-images.yaml` and `static-tiles.yaml` — so there is no
-/// `info.title` left that describes any of them. Hand-written for that
-/// reason and only that reason: a service that still owns a file keeps that
-/// file's `info` exactly as before.
+/// [`CLI_COMMAND_EXTENSION`] builds these services out of operations that
+/// actually live in other files — `sprites` from five operations in
+/// `styles.yaml`, `tilesets` from one operation each in `rastertiles.yaml`,
+/// `rasterarrays.yaml`, `tilequery.yaml` and `vectortiles.yaml` (#116),
+/// `static` from one operation each in `static-images.yaml` and
+/// `static-tiles.yaml`. None of those files' `info.title` describes the
+/// merged service, so this table is hand-written to fill that gap. A
+/// service that still owns its own file keeps that file's `info` as-is.
const MERGED_SERVICES: &[(&str, &str, &str)] = &[
(
"sprites",
@@ -730,15 +710,15 @@ const MERGED_SERVICES: &[(&str, &str, &str)] = &[
];
/// One line of help for an intermediate command group — a path segment
-/// several operations share, which is a command in the tree with no
-/// operation of its own behind it.
+/// several operations share, that shows up as a command in the tree with
+/// no operation of its own.
///
-/// Hand-written because nothing describes it: the group exists only because
-/// [`CLI_COMMAND_EXTENSION`] filed two commands under one word, and no spec
-/// has anything to say about that word. Keyed by the whole path, service
-/// included, which is how `crate::build_service_command` looks one up.
-/// `every_command_group_is_described` holds the table to the groups the
-/// surface really builds.
+/// Hand-written because nothing else describes it: the group exists only
+/// because [`CLI_COMMAND_EXTENSION`] filed two commands under one shared
+/// word, and no spec says anything about that word. Keyed by the whole
+/// path, service included — that's how `crate::build_service_command`
+/// looks it up. `every_command_group_is_described` checks this table
+/// against the groups the surface actually builds.
const COMMAND_GROUPS: &[(&str, &str)] = &[(
"styles draft",
"Work with a style's draft, the unpublished copy edits are made against",
@@ -756,25 +736,26 @@ pub fn command_group_about(path: &str) -> Option<&'static str> {
/// answers [`parse_spec`] gives.
///
/// This pass exists because an operation's service comes from
-/// [`CLI_COMMAND_EXTENSION`] rather than from the file it was parsed out of,
-/// and may name a service some *other* file is wired under — or one no file
-/// is. Which operations a service has is therefore not knowable until every
-/// file has been parsed, which is why `parse_spec` answers per file and the
-/// surface is assembled here. Three things follow:
+/// [`CLI_COMMAND_EXTENSION`], not from the file it was parsed out of — and
+/// it might name a service that some *other* file is wired under, or one no
+/// file is wired under at all. So we can't know which operations a service
+/// has until every file has been parsed. That's why `parse_spec` answers
+/// per file, and this function assembles the real surface afterward. Three
+/// consequences:
///
-/// - **A service whose operations all moved away is dropped**, rather than
-/// built as a command group with nothing in it. `maps` and `vectortiles`
-/// are exactly that today: their one operation each now targets
-/// `tilesets`. An empty group would be listed by `mapbox --help` and then
-/// refused by its own `subcommand_required(true)`.
-/// - **A service no file is wired under** takes its title and description
-/// from [`MERGED_SERVICES`].
-/// - **Order is first appearance**, across the files in
-/// [`effective_spec_entries`] order, so a service sits where its
-/// operations put it.
+/// - **A service whose operations all moved away is dropped**, instead
+/// of built as an empty command group. `maps` and `vectortiles` are
+/// exactly that today — their one operation each now targets
+/// `tilesets`. An empty group would show up in `mapbox --help` and then
+/// get refused by its own `subcommand_required(true)`.
+/// - **A service with no file wired under it** takes its title and
+/// description from [`MERGED_SERVICES`].
+/// - **Order is first appearance**, across files in
+/// [`effective_spec_entries`] order — so a service ends up wherever its
+/// operations first show up.
pub fn regroup_by_service(parsed: Vec) -> Vec {
- // Kept before `parsed` is consumed: a service that still owns a file
- // keeps that file's `info`, and the file is the only place it exists.
+ // Grabbed before `parsed` is consumed: a service that still owns a
+ // file keeps that file's `info`, which only exists in `parsed`.
let from_specs: Vec<(String, String, Option)> = parsed
.iter()
.map(|svc| (svc.name.clone(), svc.title.clone(), svc.description.clone()))
@@ -829,11 +810,11 @@ pub fn regroup_by_service(parsed: Vec) -> Vec {
/// One spec file, as far as it can be read on its own.
///
-/// The `name`, `title` and `description` are the file's; the operations may
-/// not be, since each one carries the service its own
+/// The `name`, `title` and `description` belong to the file, but the
+/// operations might not — each carries whatever service its own
/// [`CLI_COMMAND_EXTENSION`] names. [`regroup_by_service`] is what turns a
-/// list of these into the services the CLI actually builds — nothing else
-/// should treat one of these as a service.
+/// list of these into the services the CLI actually builds. Nothing else
+/// should treat one of these as a real service.
pub fn parse_spec(service_name: &str, yaml: &str) -> Result {
let doc: Value = serde_yaml::from_str(yaml)
.with_context(|| format!("Failed to parse YAML for service '{}'", service_name))?;
@@ -843,8 +824,9 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result {
.unwrap_or(service_name)
.to_string();
- // Flattened to one line, unlike a parameter's: this renders as a
- // service's `long_about`, where the spec's own wrapping buys nothing.
+ // 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.
let description = doc["info"]["description"]
.as_str()
.map(|text| first_paragraph(text).replace('\n', " "));
@@ -874,7 +856,6 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result {
for (path_key, path_item) in paths {
let path_str = path_key.as_str().unwrap_or("");
- // Collect path-level parameters (shared across methods)
let path_level_params = collect_parameters(path_item, "parameters", &doc);
for method in &["get", "post", "put", "patch", "delete"] {
@@ -899,9 +880,9 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result {
operation_id.and_then(|id| alias_for(service_name, id)),
);
- // The extension has the last word on both where the command
- // lives and what it is called; the generated name is what is
- // left when a spec declares none. See `CLI_COMMAND_EXTENSION`.
+ // The extension has the final say on both where the command
+ // lives and what it's called. The generated name is only used
+ // when a spec declares no extension. See `CLI_COMMAND_EXTENSION`.
let (service, command_path) = cli_command_target(op, operation_id)?
.unwrap_or_else(|| (service_name.to_string(), vec![generated_command]));
@@ -994,17 +975,17 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result {
/// Reads an operation's `requestBody` into the media types it declares.
///
-/// `Some` with an empty `content_types` is deliberate rather than `None`: a
-/// `requestBody` with no `content` still means the operation takes a body,
-/// which is exactly what the old `has_body` bool recorded, and
-/// [`RequestBody::accepts_json`] keeps treating it as JSON.
+/// `Some` with an empty `content_types` is deliberate, not a bug: a
+/// `requestBody` with no `content` still means the operation takes a body
+/// — the same thing the old `has_body` bool recorded — and
+/// [`RequestBody::accepts_json`] still treats it as JSON.
fn parse_request_body(op: &Value, full_spec: &Value) -> Option {
let request_body = &op["requestBody"];
// A shared body lives under `components/requestBodies` and arrives here
- // as a `$ref`. That is still a mapping, so the check below would pass it
- // through as a body with no declared type and no `required` — an
- // operation that insists on a body, described as taking an optional JSON
- // one. No bundled spec factors a body out today; one sync could.
+ // as a `$ref`, which is still a mapping — so without resolving it, the
+ // check below would treat an operation that requires a body as one
+ // with no declared type and no `required` flag. No bundled spec shares
+ // a body today, but a future sync could add one.
let request_body = request_body["$ref"]
.as_str()
.and_then(|reference| resolve_ref(full_spec, reference))
@@ -1047,10 +1028,10 @@ fn parse_request_body(op: &Value, full_spec: &Value) -> Option {
/// The multipart property that carries the uploaded files.
///
-/// Preferring a property whose schema is `format: binary` — directly or as an
-/// array's items — rather than taking the first one: a multipart body can mix
-/// files with ordinary text fields, and posting the bytes under the wrong
-/// name is a 400 that names neither.
+/// We prefer a property whose schema is `format: binary` (directly, or as
+/// an array's items) over just taking the first property. A multipart body
+/// can mix files with ordinary text fields, and posting the bytes under the
+/// wrong field name gives a 400 that doesn't name either one.
fn multipart_file_field(schema: &Value, full_spec: &Value) -> Option {
let schema = match schema["$ref"].as_str() {
Some(reference) => resolve_ref(full_spec, reference)?,
@@ -1100,21 +1081,21 @@ fn collect_parameters(node: &Value, key: &str, full_spec: &Value) -> Vec` as "see one of these"
-/// would be a lie about what the API can do.
+/// more parameter — e.g. `/styles/v1/{username}` and
+/// `/styles/v1/{username}/{style_id}`. That extra parameter has to
+/// identify an item, so account-scoping parameters don't count:
+/// `/tokens/v2` extended by `{username}` is still just another listing,
+/// not a way to look at one token. Treating it as one would wrongly offer
+/// `mapbox accounts list-tokens ` as "see one of these".
fn link_detail_operations(operations: &mut [Operation]) {
- // Only operations that are commands may be pointed at. The hint this
- // fills reaches a caller as something to run — `--schema` publishes it as
- // `detail_command` — and half the withheld and unusable sets are GETs on
- // exactly the paths this matches, so an unfiltered pairing would sooner
- // or later name a command that answers like a typo.
+ // Only real commands can be pointed at here. This hint reaches a
+ // caller as something to run — `--schema` publishes it as
+ // `detail_command` — and many withheld or unsupported operations are
+ // GETs on exactly these kinds of paths. Without this filter, we could
+ // point someone at a command that doesn't actually work.
let candidates: Vec<(String, String)> = operations
.iter()
.filter(|op| op.method == "GET" && op.is_exposed())
@@ -1124,9 +1105,9 @@ fn link_detail_operations(operations: &mut [Operation]) {
for op in operations.iter_mut().filter(|op| op.method == "GET") {
let prefix = format!("{}/{{", op.path_template);
op.detail = candidates.iter().find_map(|(path, command)| {
- // The name comes from the path, not from the parameter list: the
- // path is what defines the extension, and some specs describe a
- // placeholder without declaring a parameter for it.
+ // The name comes from the path, not the parameter list — some
+ // specs use a placeholder in the path without ever declaring a
+ // matching parameter for it.
let name = path
.strip_prefix(&prefix)
.and_then(|rest| rest.strip_suffix('}'))?;
@@ -1140,11 +1121,11 @@ fn link_detail_operations(operations: &mut [Operation]) {
});
}
- // The same pairing read the other way, for the failure that wants it: a
- // 404 from a detail operation is most often an id that does not exist,
- // and the listing is where the ids that do exist come from. Only a
- // listing that is itself a command may be named — the pass above pairs on
- // paths alone, so an unexposed GET can hold a `detail` link while not
+ // The same pairing, read backwards, for when it matters most: a 404
+ // from a detail operation usually means the id doesn't exist, and the
+ // listing is where valid ids come from. Only a listing that's itself a
+ // real command gets named here — the pass above pairs on paths alone,
+ // so an unexposed GET could otherwise hold a `detail` link while not
// being runnable itself.
let listings: Vec<(String, ListingOperation)> = operations
.iter()
@@ -1162,8 +1143,9 @@ fn link_detail_operations(operations: &mut [Operation]) {
.collect();
for op in operations.iter_mut() {
- // Matched on the whole command, service included, rather than on the
- // last word: `styles get` and `styles draft get` share one.
+ // Matched on the whole command, service included, not just the
+ // last word — `styles get` and `styles draft get` both end in
+ // `get`.
let command = op.command();
op.listing = listings
.iter()
@@ -1172,15 +1154,17 @@ fn link_detail_operations(operations: &mut [Operation]) {
}
}
-/// Everything up to the first blank line, wrapped as the spec wrapped it.
+/// Everything up to the first blank line, with line breaks kept as the
+/// spec wrote them.
///
-/// Spec prose puts the definition first and the reference material after a
-/// break — the options table, the per-country notes, the worked examples.
-/// The first paragraph is the part that describes the thing.
+/// Spec prose usually puts the actual definition first, then reference
+/// material after a blank line — an options table, per-country notes,
+/// worked examples. The first paragraph is the part that describes the
+/// thing itself.
///
-/// The line breaks inside it are kept. They are what `--help` has always
-/// rendered, so dropping them would reflow four arguments' help for no
-/// reason, and they are the only structure the bulleted ones have.
+/// We keep the line breaks because `--help` has always rendered them, and
+/// dropping them would needlessly reflow a few arguments' help text — plus
+/// they're the only structure that bulleted descriptions have.
fn first_paragraph(text: &str) -> String {
text.lines()
.take_while(|line| !line.trim().is_empty())
@@ -1204,17 +1188,18 @@ fn scalar_to_string(value: &Value) -> Option {
fn parse_parameter(val: &Value) -> Option {
let name = val["name"].as_str()?.to_string();
let required = val["required"].as_bool().unwrap_or(false);
- // The first paragraph, the same rule the service description above uses.
+ // The first paragraph, same rule as the service description above.
//
- // It used to be cut at the first `.`, which is fine for a line of help
- // and wrong for `--schema`, where it is the field a caller reasons from:
- // that cut lands inside `username.tileset-id`, inside `(range -85.0511,
- // 85.0511)` and inside `e.g.`, publishing a truncated identifier and a
- // wrong bound. Keeping the whole text instead was the other extreme —
- // `geocoder --types` alone is 4.6 KB of feature-type reference, and the
- // schema is read on every call. A paragraph keeps every case the cut
- // broke and leaves the appendices behind. `crate::first_sentence`
- // shortens it further for help, which renders exactly as it always has.
+ // This used to cut at the first `.`, which works fine for a line of
+ // help but breaks `--schema`, where callers actually reason from this
+ // field. That cut landed inside `username.tileset-id`, inside `(range
+ // -85.0511, 85.0511)`, and inside `e.g.` — publishing a truncated
+ // identifier and a wrong bound. Keeping the whole text was the other
+ // 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"];
@@ -1226,11 +1211,11 @@ fn parse_parameter(val: &Value) -> Option {
_ => None,
};
- // Numbers and booleans count. Keeping only the strings silently dropped
- // `tilesize: enum [256, 512]`, the one non-string enum among the
- // parameters in the bundled specs — so the CLI accepted `300`, sent it,
- // and let the API refuse it. Everything downstream wants the value as
- // text anyway: it is going into a URL.
+ // Numbers and booleans count too. Keeping only strings used to
+ // silently drop `tilesize: enum [256, 512]` — the one non-string enum
+ // among the bundled specs' parameters — so the CLI would accept `300`,
+ // send it, and let the API reject it instead. Everything downstream
+ // wants the value as text anyway, since it's going into a URL.
let enum_values: Vec = schema["enum"]
.as_sequence()
.map(|values| values.iter().filter_map(scalar_to_string).collect())
@@ -1464,13 +1449,13 @@ paths:
);
}
- /// A listing that is not itself a command must never be named as one.
- /// This fixture gives a listing-shaped operation an operationId that's
- /// in `UNSUPPORTED_OPERATIONS` (`getFontCoverage`, `fonts:metadata` —
- /// no login can carry that scope), so it's filtered out of the surface —
- /// and a 404 pointed at it would tell the caller to run something that
- /// does not exist. Which entry the fixture borrows doesn't matter; only
- /// that one still needs a scope DCR won't grant. `listFonts` served this
+ /// A listing that isn't itself a command must never be named as one.
+ /// This fixture gives a listing-shaped operation an operationId from
+ /// `UNSUPPORTED_OPERATIONS` (`getFontCoverage`, needing a scope no
+ /// login can carry), so it's filtered out of the surface. Pointing a
+ /// 404 at it would otherwise tell the caller to run something that
+ /// doesn't exist. Which entry we borrow doesn't matter, as long as it
+ /// still needs a scope DCR won't grant — `listFonts` served this
/// purpose until `fonts:list` became registrable.
#[test]
fn an_unusable_listing_is_never_named() {
@@ -1746,12 +1731,12 @@ paths:
entries.iter().map(|entry| entry.name).collect()
}
- /// The command a merged entry would actually generate, which is the only
- /// form of "which spec won" that a caller can see.
+ /// The command a merged entry would actually generate — the only
+ /// visible sign of "which spec won".
///
- /// Named apart from the real `command_names` (which computes one
- /// operation's name and aliases): a glob `use super::*` would otherwise
- /// let this test-only helper shadow it.
+ /// Named differently from the real `command_names` (which computes one
+ /// operation's name and aliases) so `use super::*` doesn't let this
+ /// test-only helper shadow it.
fn merged_command_names(entries: &[SpecEntry], name: &str) -> Vec {
let entry = entries
.iter()
@@ -1844,13 +1829,13 @@ paths:
}
// `build_app` registers six hand-written top-level commands
- // alongside the generated ones — `auth` has no `COMMAND` const to
- // borrow, the rest do. A spec table claiming any of these names
- // collides the same way two same-named spec entries would, and
- // `merge_entries` has no way to catch a clash with a name outside
- // its own tables. Pulling from each module's `COMMAND` const, rather
- // than hard-coding the string a second time, means a seventh
- // hand-written command is covered the day it lands.
+ // alongside the generated ones (`auth` has no `COMMAND` const to
+ // borrow; the rest do). A spec table claiming any of these names
+ // would collide the same way two same-named spec entries would,
+ // and `merge_entries` has no way to catch a clash outside its own
+ // tables. We pull the name from each module's `COMMAND` const
+ // instead of hard-coding the string again, so a seventh
+ // hand-written command is covered automatically once it lands.
for reserved in [
"auth",
crate::generate_skills::COMMAND,
@@ -1868,17 +1853,18 @@ paths:
/// Every [`COMMAND_ALIASES`] row reaches an operation that exists.
///
- /// `alias_for` answers `None` for a row whose `service` or `operationId`
- /// is misspelled, exactly as it does for the operations no row mentions —
- /// so the alias silently never appears, the command keeps its generated
- /// name, and nothing else notices. `tests/api_command_surface.rs` pins
- /// the surface as it is, which means a row that never worked is pinned
- /// as working. This is the check that a row does something.
+ /// `alias_for` returns `None` for a row with a misspelled `service` or
+ /// `operationId` — the exact same result as for an operation no row
+ /// mentions at all. So the alias would silently never appear, the
+ /// command would keep its generated name, and nothing would notice.
+ /// Worse, `tests/api_command_surface.rs` would then pin that broken row
+ /// as if it worked. This test exists to catch that: does each row
+ /// actually do something?
///
- /// Against the effective (merged) specs rather than a fixture, because
- /// the thing that goes wrong is a row pointing at an `operationId` those
- /// specs do not have — either mistyped, or renamed upstream by a later
- /// sync.
+ /// Runs against the effective (merged) specs, not a fixture, because
+ /// what actually breaks is a row pointing at an `operationId` those
+ /// specs don't have — either a typo, or a rename upstream that a later
+ /// sync picked up.
#[test]
fn every_command_alias_names_a_real_operation() {
let effective = effective_spec_entries();
@@ -1891,9 +1877,10 @@ paths:
});
let svc = parse_spec(entry.name, entry.yaml).expect("bundled spec parses");
- // The alias arrives as the command's own name or as one it
- // answers to, depending on the row — either way, exactly one
- // operation carries it, and none does if the row is dead.
+ // The alias shows up either as the command's real name or as
+ // one it also answers to, depending on the row. Either way,
+ // exactly one operation should carry it — zero if the row is
+ // dead.
let carriers: Vec<&str> = svc
.operations
.iter()
@@ -1917,25 +1904,24 @@ paths:
}
}
- /// A merge with a literal empty custom table — not `CUSTOM_SPEC_ENTRIES`,
- /// which now has `search` in it — changes nothing about the mapbox table:
- /// not order, not content, not which names appear. The general case
- /// `a_mapbox_service_with_no_override_passes_through_untouched` above
- /// exercises with a real override; this is its degenerate edge, pinned on
- /// the shipped table itself.
+ /// Merging against a truly empty custom table (not `CUSTOM_SPEC_ENTRIES`,
+ /// which has `search` in it) should change nothing at all. The test
+ /// above, `a_mapbox_service_with_no_override_passes_through_untouched`,
+ /// covers the general case with a real override; this is the edge case
+ /// with none, pinned against the real shipped table.
#[test]
fn merging_an_empty_custom_table_changes_nothing() {
let merged = merge_entries(MAPBOX_SPEC_ENTRIES, &[]);
assert_eq!(names(&merged), names(MAPBOX_SPEC_ENTRIES));
}
- /// The one duplicate shape `merge_entries` used to swallow with no
- /// trace: two `CUSTOM_SPEC_ENTRIES` entries for a name that also exists
- /// in `MAPBOX_SPEC_ENTRIES`. Both the first-match `find` and the
+ /// One duplicate shape `merge_entries` used to swallow silently: two
+ /// `CUSTOM_SPEC_ENTRIES` entries with the same name that also exists in
+ /// `MAPBOX_SPEC_ENTRIES`. Both the first-match `find` and the
/// unmatched-name filter treat this as an ordinary override, so the
- /// output has no repeated name and every mapbox name is present — the
- /// merged-list assertions above cannot see it. Only a table-level check
- /// catches it, which is what this exercises.
+ /// output looks fine — no repeated name, every mapbox name present —
+ /// and the merged-list assertions above can't catch it. Only a
+ /// table-level check like this one can.
#[test]
#[should_panic(expected = "CUSTOM_SPEC_ENTRIES lists `search` more than once")]
fn a_repeated_custom_name_panics_instead_of_silently_dropping_one() {
@@ -1945,9 +1931,9 @@ paths:
);
}
- /// Both halves of `show_generated_name`, which no row in the real table
- /// can cover: every row says `false` today, so the `true` arm would ship
- /// having never run.
+ /// Both values of `show_generated_name`. No row in the real table
+ /// exercises `true` — they're all `false` today — so this test covers
+ /// it directly.
#[test]
fn a_visible_alias_keeps_the_generated_name_and_a_hidden_one_replaces_it() {
let generated = || "get-v4tilesets-tilequery-lon-lat-json".to_string();
diff --git a/tests/docs_contract.rs b/tests/docs_contract.rs
index c23eb8c..87b9f66 100644
--- a/tests/docs_contract.rs
+++ b/tests/docs_contract.rs
@@ -1,42 +1,40 @@
//! Holds `docs/commands.md` to the surface the binary reports.
//!
-//! The surface itself cannot drift from the specs. The spec tables in
-//! `src/spec.rs` compile them in with `include_str!`, so a spec change
-//! rebuilds the commands, a spec renamed upstream is a broken build, and a
-//! new parameter named after a global fails `no_generated_flag_shadows_a_global`.
-//! `docs/commands.md` is the half with none of that: nothing in this repo
-//! writes it, and its **Parameters** tables were transcribed from the specs
-//! by hand. So the drift that actually happens runs one way — a spec moves,
-//! the CLI follows it for free, and the page goes on describing a CLI that no
-//! longer exists.
+//! The surface itself can't drift from the specs — `src/spec.rs` compiles
+//! them in with `include_str!`, so a spec change rebuilds the commands, a
+//! spec renamed upstream breaks the build, and a new parameter named after
+//! a global fails `no_generated_flag_shadows_a_global`. `docs/commands.md`
+//! has none of that: nothing writes it, and its **Parameters** tables were
+//! transcribed from the specs by hand. So drift only ever runs one way — a
+//! spec moves, the CLI follows for free, and the page keeps describing a
+//! CLI that no longer exists.
//!
-//! Nothing here reaches the network, and that is the point rather than a
-//! detail. The other way to notice a stale page is a scheduled job that runs
-//! the live API and diffs the captures, which would need a standing secret in
-//! a repo whose workflows hold none, would report days after the change, and
-//! would be monitoring an API this repo does not own. What is checkable
-//! without a token, in `cargo test`, at the moment the change is made, is
-//! whether the page has fallen behind our own binary. That is all this file
-//! claims to check.
+//! Nothing here reaches the network, on purpose. The alternative — a
+//! scheduled job that runs the live API and diffs the captures — would
+//! need a standing secret in a repo whose workflows hold none, would
+//! report days after the change, and would be monitoring an API this repo
+//! doesn't own. What's checkable without a token, in `cargo test`, right
+//! when the change is made, is whether the page has fallen behind our own
+//! binary. That's all this file claims to check.
//!
-//! Three things it deliberately does not do, written down so the next reader
-//! does not have to re-derive the scope:
+//! Three things it deliberately doesn't do, written down so the next
+//! reader doesn't have to re-derive the scope:
//!
-//! * It does not enforce every flag. A flag the page declares page-wide is
-//! exempted for all 60 commands, not only for the ones that actually
-//! share it, so 93 of the 176 flag-bearing arguments are enforced and 83
-//! are not. `--data`, `--file` and `--dry-run` are the page-wide
-//! declarations that are *not* in `global_options`: the first two are on
-//! a handful of operations and the third on 24 of them.
-//! * It does not ask for `--dry-run` section by section, which is what
-//! would lift that number. Rejected: the page's design is to state a
-//! page-wide fact once, and 24 near-identical paragraphs is the
-//! duplication that design exists to avoid. Where a reader needs the
-//! split spelled out — the four `auth` commands, three of which take the
-//! flag and one of which does not — the page says so in prose.
-//! * It does not check the captured **Outputs** blocks. They are the half
-//! no test can reach: bytes a real account returned once, a dated
-//! snapshot the page labels as one, re-taken by hand.
+//! * It doesn't enforce every flag. A page-wide flag is exempted for all
+//! 60 commands, not just the ones that actually share it, so 93 of 176
+//! flag-bearing arguments are enforced and 83 aren't. `--data`,
+//! `--file` and `--dry-run` are page-wide but *not* in
+//! `global_options`: the first two apply to only a handful of
+//! operations, the third to 24 of them.
+//! * It doesn't check `--dry-run` section by section, which would raise
+//! that number. Rejected on purpose: the page states a page-wide fact
+//! once, and 24 near-identical paragraphs is exactly the duplication
+//! that design avoids. Where a split needs spelling out — the four
+//! `auth` commands, three of which take the flag and one of which
+//! doesn't — the page says so in prose instead.
+//! * It doesn't check the captured **Outputs** blocks. Those are the
+//! half no test can reach: bytes a real account returned once, a
+//! dated snapshot re-taken by hand.
use std::collections::{BTreeMap, BTreeSet};
use std::path::PathBuf;
@@ -49,7 +47,7 @@ use serde_json::Value;
const PAGE: &str = "docs/commands.md";
/// A config directory of this test binary's own. See the same function in
-/// `output_contract.rs` for why `--profile` alone is not isolation.
+/// `output_contract.rs` for why `--profile` alone isn't isolation.
fn sandbox_home() -> PathBuf {
let home = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("docs-contract-home");
std::fs::create_dir_all(&home).expect("create sandbox home");
@@ -58,7 +56,7 @@ fn sandbox_home() -> PathBuf {
/// The whole surface, from the binary that was just built.
///
-/// The developer's own environment is cleared for the reason it is cleared in
+/// The developer's environment is cleared for the same reason as in
/// `schema_contract.rs`: a token in the running shell would let a mistake
/// here reach the API, and this file has no business touching it.
fn schema() -> Value {
@@ -83,8 +81,8 @@ fn schema() -> Value {
serde_json::from_slice(&out.stdout).expect("`mapbox --schema` prints one JSON document")
}
-/// Reached through `CARGO_MANIFEST_DIR` rather than a path relative to the
-/// working directory, which `cargo test` does not promise.
+/// Reached through `CARGO_MANIFEST_DIR` instead of a path relative to the
+/// working directory, which `cargo test` doesn't guarantee.
fn page() -> String {
let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(PAGE);
std::fs::read_to_string(&path).unwrap_or_else(|e| panic!("read {}: {e}", path.display()))
@@ -100,16 +98,16 @@ fn name(command: &Value) -> &str {
/// Every flag spelling in one line of the page, added to `found`.
///
-/// Hand-rolled rather than a regex so the test adds no dependency, but the
-/// character class is the part worth getting right: it has to accept
-/// uppercase. A `[a-z-]+` scanner truncates `--sourceEncoding` to `--source`
-/// and `--mapboxGLVersion` to `--mapbox`, which then read as two flags the
-/// page had failed to mention — drift the scanner invented rather than found.
+/// Hand-rolled instead of a regex to avoid a dependency, but the character
+/// class matters: it has to accept uppercase. A `[a-z-]+` scanner would
+/// truncate `--sourceEncoding` to `--source` and `--mapboxGLVersion` to
+/// `--mapbox`, reading as flags the page failed to mention — drift the
+/// scanner invented, not found.
///
-/// Over-matching is safe in the other direction and not worth guarding: this
-/// runs only over the page, and the set it fills is only ever asked whether
-/// it holds a flag some command actually declares. `-rw-r--r--` in a captured
-/// `ls -l` line yields `--r--`, which no command will ever be looking for.
+/// Over-matching is safe and not worth guarding against: this only runs
+/// over the page, and the resulting set is only ever checked against
+/// flags some real command declares. `-rw-r--r--` in a captured `ls -l`
+/// line yields `--r--`, which no command will ever look for.
fn flags_in(line: &str, found: &mut BTreeSet) {
let bytes = line.as_bytes();
for start in 0..bytes.len().saturating_sub(2) {
@@ -126,9 +124,9 @@ fn flags_in(line: &str, found: &mut BTreeSet) {
/// The page's lines, each paired with whether it sits inside a fenced block.
///
-/// Both parsers below need the distinction and disagree about what to do with
-/// it, which is why it is answered once here rather than in each. A fence
-/// delimiter counts as inside: it is neither a heading nor a flag.
+/// Both parsers below need this and treat it differently, so it's answered
+/// once here instead of in each. A fence delimiter counts as inside — it's
+/// neither a heading nor a flag.
fn lines(page: &str) -> impl Iterator {
let mut fenced = false;
page.lines().map(move |line| {
@@ -146,12 +144,12 @@ fn heading_depth(line: &str) -> Option {
(hashes > 0 && line.as_bytes().get(hashes) == Some(&b' ')).then_some(hashes)
}
-/// The command a `### ` heading is about, if it is about one.
+/// The command a `### ` heading is about, if it's about one.
///
/// The page writes the tileset proxy's forwarded argv into its heading —
-/// ``### `mapbox tilesets-cli ` `` — so a trailing placeholder is
-/// dropped before matching. Cutting at `<` is enough because no command name
-/// holds one.
+/// ``### `mapbox tilesets-cli ` `` — so a trailing placeholder gets
+/// dropped before matching. Cutting at `<` is enough, since no command
+/// name contains one.
fn command_in_heading(line: &str) -> Option {
let quoted = line.strip_prefix("### `")?.split('`').next()?;
let named = quoted.split('<').next().unwrap_or(quoted).trim();
@@ -163,25 +161,24 @@ fn command_in_heading(line: &str) -> Option {
///
/// The whole section counts — table, prose and worked examples alike, not
/// only the `#### Parameters` table. The page legitimately introduces a flag
-/// in prose (forward's structured input is a paragraph naming nine of
-/// them, and `--fresh` is one sentence) or in an example, and a check that
-/// read only the tables would be enforcing a house style rather than finding
-/// drift.
+/// in prose (forward's structured input is a paragraph naming nine of them)
+/// or in an example, and a check that read only the tables would be
+/// enforcing a house style rather than finding drift.
///
-/// A `## ` service heading closes a section as surely as the next `### `
-/// does. Missing that reads a service's whole trailing prose as part of its
-/// last command's section, and then a real gap anywhere after the first
-/// command of a service goes unreported.
+/// A `## ` service heading closes a section, same as the next `### `
+/// does. Missing that would read a service's whole trailing prose as part
+/// of its last command's section, hiding a real gap anywhere after the
+/// service's first command.
fn sections(page: &str) -> BTreeMap> {
let mut sections = BTreeMap::new();
let mut current: Option = None;
for (line, fenced) in lines(page) {
- // `#### Parameters` and `#### Outputs` are inside a section, so only
- // the levels above one end it — and a `#`-prefixed line inside a
- // captured block is output, not a heading. No capture holds one
- // today; tracking it means the day one does, the section it sits in
- // does not silently close and take its remaining flags with it.
+ // `#### Parameters` and `#### Outputs` are inside a section, so
+ // only levels above that end it. A `#`-prefixed line inside a
+ // captured block is output, not a heading — no capture holds one
+ // today, but tracking `fenced` means the section won't silently
+ // close if one ever does.
if !fenced && heading_depth(line).is_some_and(|depth| depth <= 3) {
current = command_in_heading(line);
if let Some(command) = ¤t {
@@ -199,15 +196,15 @@ fn sections(page: &str) -> BTreeMap> {
/// The flags the page declares once, for every command.
///
-/// Read out of the page rather than listed here: the table under `### What
-/// every API command takes`, and the prose below it that declares `--data`
-/// and `--file`. Parsing them from the page means promoting a flag to a
-/// global stays one edit, in the place a reader looks for it, and this test
-/// follows that edit instead of having to be told about it.
+/// Read out of the page instead of listed here — the table under `### What
+/// every API command takes`, plus the prose below it declaring `--data`
+/// and `--file`. Parsing them from the page means promoting a flag to
+/// global stays a one-edit change, right where a reader looks for it, and
+/// this test just follows along.
///
-/// Fenced blocks in that section are skipped. The two example command lines
-/// there reach for `--q`, and a flag written into an example is being used,
-/// not declared for everything.
+/// Fenced blocks in that section are skipped: the example command lines
+/// there use `--q`, and a flag written into an example is being used, not
+/// declared for everything.
fn declared_once_for_every_command(page: &str) -> BTreeSet {
let mut flags = BTreeSet::new();
let mut inside = false;
@@ -225,9 +222,10 @@ fn declared_once_for_every_command(page: &str) -> BTreeSet {
}
}
- // A heading that gets reworded leaves this empty, and an empty exemption
- // set does not fail loudly — it holds every command to flags the page
- // does answer for elsewhere, and buries the real report under them.
+ // If the heading gets reworded, this comes back empty — and an empty
+ // exemption set fails quietly, holding every command to flags the
+ // page does answer for elsewhere and burying the real report under
+ // them.
assert!(
flags.contains("--data") && flags.contains("--file"),
"the `### What every API command takes` section parsed to {flags:?}, and \
@@ -239,35 +237,33 @@ fn declared_once_for_every_command(page: &str) -> BTreeSet {
/// Which spec revision the binary was built from, for a failure message.
///
-/// Both assertions below compare the page against the *binary*, and the
-/// binary is built from `openapi/` — a vendored copy that only moves when
-/// a maintainer regenerates it. So when they disagree there are two
-/// candidate culprits, and the message used to name only one of them: the
-/// page.
+/// Both assertions below compare the page against the *binary*, which is
+/// built from `openapi/` — a vendored copy that only moves when a
+/// maintainer regenerates it. So when they disagree, there are two
+/// possible culprits, but the message used to name only one: the page.
///
-/// That is not a hypothetical. Specs three weeks old reported
-/// `sources get-datasource` as a command the CLI no longer has — the
-/// operation had been briefly absent upstream and was restored days later —
+/// That's not hypothetical. Three-week-old specs once reported
+/// `sources get-datasource` as a command the CLI no longer had — the
+/// operation had briefly disappeared upstream and come back days later —
/// and the failure read as "delete the section". Two reviewers and a
-/// maintainer took it at face value, while `build.rs` printed a staleness
-/// warning on every one of those builds.
+/// maintainer took that at face value, even though `build.rs` printed a
+/// staleness warning on every one of those builds.
///
-/// So this says which revision the binary was built from, and — the half
-/// that matters — *rules the specs out* when they are fresh, leaving the page
-/// as the only remaining explanation.
+/// So this names the revision the binary was built from, and — the part
+/// that matters — *rules out* the specs when they're fresh, leaving the
+/// page as the only remaining explanation.
///
-/// The advice changed when the specs moved in-tree: it used to say pull the
-/// sibling checkout, which an external clone has no way to do. Now it says
-/// plainly that `openapi/` is vendored and regenerating it is a
-/// maintainer-only step, rather than pointing at tooling this checkout
-/// doesn't have.
+/// The advice changed once specs moved in-tree: it used to say pull the
+/// sibling checkout, which an external clone can't do. Now it just says
+/// `openapi/` is vendored and regenerating it is a maintainer-only step,
+/// instead of pointing at tooling this checkout doesn't have.
fn spec_revision_note() -> String {
let commit = option_env!("MAPBOX_SPEC_COMMIT");
let age: Option = option_env!("MAPBOX_SPEC_AGE_DAYS").and_then(|days| days.parse().ok());
- // Matches `STALE_AFTER_DAYS` in build.rs. Duplicated rather than shared
- // because build.rs cannot export a constant to a test, and a wrong number
- // here costs a wrong hint rather than a wrong result.
+ // Matches `STALE_AFTER_DAYS` in build.rs. Duplicated, not shared,
+ // because build.rs can't export a constant to a test — a wrong number
+ // here just costs a wrong hint, not a wrong result.
const STALE_AFTER_DAYS: u64 = 14;
match (commit, age) {
@@ -321,17 +317,17 @@ fn the_page_has_a_section_for_every_command_and_for_nothing_else() {
);
}
-/// One direction, deliberately: a flag the command declares has to be named
-/// in its section, but a flag named in a section need not belong to that
-/// command.
+/// One direction, deliberately: a flag the command declares must be named
+/// in its section, but a flag named in a section doesn't have to belong to
+/// that command.
///
-/// The reverse is noise rather than drift. A section's prose legitimately
-/// reaches for another command's flags — `styles delete` explains itself in
-/// terms of `styles list --deleted`, which is `styles list`'s flag and not
-/// its own — and a captured `ls -l` line reads `-rw-r--r--` as a flag called
-/// `--r--`. A prototype of this check asserted both ways and reported those
-/// as drift; none of them was, and a check whose failures have to be triaged
-/// by hand is one nobody reads.
+/// The reverse is noise, not drift. A section's prose legitimately
+/// reaches for another command's flags — `styles delete` explains itself
+/// via `styles list --deleted`, which belongs to `styles list`, not
+/// itself — and a captured `ls -l` line reads `-rw-r--r--` as a flag
+/// called `--r--`. An earlier version of this check asserted both
+/// directions and reported those as drift; none of them were, and a check
+/// whose failures need manual triage is one nobody reads.
#[test]
fn every_flag_a_command_takes_is_named_in_its_own_section() {
let schema = schema();
@@ -351,8 +347,8 @@ fn every_flag_a_command_takes_is_named_in_its_own_section() {
let mut unmentioned = Vec::new();
for command in commands(&schema) {
let command_name = name(command);
- // A command with no section at all is the other test's report, not a
- // flag gap on every one of its arguments.
+ // A command with no section at all is the other test's report to
+ // make, not a flag gap on every one of its arguments.
let Some(named) = sections.get(command_name) else {
continue;
};
@@ -361,7 +357,7 @@ fn every_flag_a_command_takes_is_named_in_its_own_section() {
.map(Vec::as_slice)
.unwrap_or_default()
{
- // Positionals have no flag; the page describes those in prose.
+ // Positionals have no flag — the page describes those in prose.
let Some(flag) = argument["flag"].as_str() else {
continue;
};
diff --git a/tests/fixtures/api_command_surface.txt b/tests/fixtures/api_command_surface.txt
index ba4e9a1..9dd4675 100644
--- a/tests/fixtures/api_command_surface.txt
+++ b/tests/fixtures/api_command_surface.txt
@@ -26,7 +26,6 @@ mapbox styles draft update | aliases: (none)
mapbox styles get | aliases: (none)
mapbox styles list | aliases: (none)
mapbox styles update | aliases: (none)
-mapbox tilesets get-mrt | aliases: (none)
mapbox tilesets get-mvt | aliases: (none)
mapbox tilesets get-tile | aliases: (none)
mapbox tilesets query | aliases: (none)