Skip to content

Add mapbox config list/unset alongside get/set - #39

Closed
mattpodwysocki wants to merge 1 commit into
feat/106-persisted-configfrom
feat/177-config-list-unset
Closed

mattpodwysocki wants to merge 1 commit into
feat/106-persisted-configfrom
feat/177-config-list-unset

Conversation

@mattpodwysocki

Copy link
Copy Markdown
Contributor

Summary

Stacked on #35 (mapbox config get/set) — base branch is feat/106-persisted-config, so this diff is just the incremental change. Rebase onto main once #35 merges.

  • mapbox config list: every setting and its current value in one call, rather than one key at a time.
  • mapbox config unset <key>: clears a key back to "never set" rather than writing its current default value explicitly. The two read identically through resolve() today, but they're not the same fact on disk — a later default change reaches a cleared key and not one a caller pinned to the old default on purpose.
  • get/list now share one resolve() so the two can't answer a key differently; unset gets a clear() counterpart to set's Some(enabled).

Test plan

  • cargo build, cargo fmt, cargo clippy --all-targets -- -D warnings, cargo test — all clean
  • New unit tests in src/config.rs for resolve/clear
  • New integration tests in tests/config.rs: list reports the one known key at its default and after a set, in both text and JSON; unset is proven to remove the key from the file rather than just re-writing its default value (checked against the raw file contents, not just the CLI's own read-back)
  • docs/commands.md and CHANGELOG updated

get/set answer one key at a time; list answers all of them in one call,
each falling back to its default the same way get does — worth having
before a second setting makes the gap between "check everything" and
"check one key" visible.

unset clears a key back to "never set" rather than writing its current
default value explicitly. The two read identically through resolve() today,
but they are not the same fact on disk: a later default change reaches a
cleared key, and does not reach one a caller pinned to the old default on
purpose. clear() is unset's counterpart to set()'s Some(enabled), and
get/list now share one resolve() so the two cannot answer a key
differently.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mattpodwysocki
mattpodwysocki requested a review from a team as a code owner September 23, 2026 03:38
@zmofei
zmofei added this pull request to stack #41 September 23, 2026 11:49

@zmofei zmofei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small, well-tested PR overall — the write/persist logic, schema/completion generalization, and new tests all check out, and I didn't find any correctness bugs.

One thing worth fixing before merge:

  • resolve(), clear(), and set() each have their own match key { UPDATE_CHECK_KEY => ..., _ => unreachable!(...) } block. Adding a second config key now means updating three separate match sites (plus KEYS), and missing one still compiles — it just panics at runtime instead. A single per-key table (key, getter, setter) would make adding a setting a one-line change and remove that footgun.

Two minor nits, non-blocking:

  • tests/config.rs's module doc still says it covers get/set only, but this PR adds list/unset tests to the same file.
  • list() calls resolve() twice per key (once for entries, once for text); a single pass would avoid the duplicate work as more keys are added.

@zmofei
zmofei removed this pull request from stack #41 September 23, 2026 11:58
@mattpodwysocki
mattpodwysocki deleted the branch feat/106-persisted-config September 23, 2026 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants