docs(context): correct gotchas 15 and 16 and drop the duplicate 8jk6.6 kb fact
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
committed by
Edward Langley
parent
dd1bf96e00
commit
942888e0dc
+4
-4
@@ -175,8 +175,8 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
12. **Panel keymaps come from `PanelKeymap`.** The three panel modes share a spine (Esc/Tab→Normal, arrows and j/k→`move-panel-cursor <panel>`, every panel's toggle key, `:`→command mode) built by `PanelKeymap::new(panel)`; the keys that act on the panel's contents arrive through `.bind`/`.bind_args`/`.delete` because they vary in shape, not just command name (improvise-4pg). `Panel::ALL`/`name()`/`key()` in `ui/app.rs` drive the toggle keys for Normal and all three panels, so a fourth panel reaches every keymap from one `ALL` entry. `name()` must spell what `parse_panel` accepts.
|
||||
13. **Prefix keys are stored as an address, not a keymap.** `ViewState::transient_prefix` holds the `(KeyCode, KeyModifiers)` that opened a prefix; `KeymapSet::prefix_keymap` resolves it back to the sub-keymap, walking parents so `RecordsNormal` reaches Normal's `g`/`y`/`Z` (improvise-jc8) — that is the command-layer wrapper; the resolution itself is `Keymaps::prefix_keymap` in `improvise-protocol/src/keymap.rs`. `ViewState` must stay free of `improvise-command` types — it is destined for the protocol crate, which `improvise-command` will depend on. Resolution assumes prefixes are **flat**; `prefix_keymaps_are_flat` fails the build if a nested one is added.
|
||||
14. **Keybindings are per-mode.** `ModeKey::from_app_mode()` resolves the mode; Normal + `search_mode=true` maps to `SearchMode`. Minibuffer modes bind Enter/Esc as `Binding::Sequence` so `clear-buffer` fires alongside commit/cancel. Hints are checked against the keymaps: `every_hint_key_resolves_in_its_mode` (`command/keymap.rs`) splits each `AppMode::hint_text()` and requires every key it names to resolve in that mode's map, because `only_quit_has_no_hint` asserts only that a hint is *present*. It checks which keys are named, not what they do. Its tokenizer knows the hint dialect explicitly (named keys, `Ctrl-<c>`, `F/C/V` alternatives, character runs like `hjkl`/`[:]`, the em-dash prose tail, CommandMode's `:q` ex-commands) and panics on a token it cannot classify rather than skipping it.
|
||||
15. **Registry names come from prototypes, not strings.** Every `register*` call reads the name off a prototype `Cmd` instance, so a typo is impossible (improvise-9cn) — there is no name literal in `registry.rs`. What types can't reach: four commands name themselves off a field (`ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`, `CommitAndAdvance`), so a prototype can name the wrong *variant* and still compile. That shows up as a **duplicate** registration, not a mismatch — the wrong-variant entry collides with its sibling and the sibling's name vanishes. `registered_names_are_unique` catches it; `every_bound_command_name_is_registered` catches the keymap side, and checks **both** halves of `dispatch`'s `registry.interactive(name, args, ctx).ok()?` — the name resolves *and* the args parse — because improvise-s13 moved `enter-mode ["export-prompt"]`/`["formula-edit"]` out of the nullary class, where a typo'd mode name compiles into a dead key. It builds the `CmdContext` in each keymap's own `AppMode`, since `commit-buffer item` reads `ctx.mode`.
|
||||
16. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic. Its counterpart on the effect side is `simple_effect!` in `ui/effect.rs`, which declares a one-field newtype that writes one place on the `App`. Its **kind argument is mandatory** — `simple_effect!(Name(T) => |app| place, EffectKind::ViewOnly)` — even though every call site so far answers `ViewOnly`: hard-coding it reintroduced exactly the default gotcha 3 does without, since `$place` is an unconstrained expression and a one-line `active_view_mut().number_format` setter would have reported `ViewOnly` with the suite green (improvise-8fy). A wrapper whose effect names a category, item, group or view returns `unless_unknown(needs, effect)` rather than the bare effect (gotcha 51). Likewise, mode entry has no per-mode shim commands — bind `enter-mode <mode-name>` (improvise-s13); the name is decoded by `parse_mode_name` in `registry.rs`, and `EnterMode` clears the mode's minibuffer on the way in.
|
||||
15. **Registry names come from prototypes, not strings.** Every `register*` call reads the name off a prototype `Cmd` instance, so a typo is impossible (improvise-9cn) — so no `register*` call spells a name. Name literals do remain in `registry.rs`, and nothing but the keymap test guards them: the `Move` prototypes carry `cmd_name: "move-selection"` / `"scroll-rows"` / `"page-scroll"` (the four `jump-*` names go through `reg_jump!`), and four `r.alias` calls spell an alias and its target (`add-cat`, `formula`, `add-view`, `q!`). What types can't reach: five commands name themselves off a field (`Move` off `cmd_name`, `ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`, `CommitAndAdvance`), so a prototype can name the wrong *variant* and still compile. That shows up as a **duplicate** registration, not a mismatch — the wrong-variant entry collides with its sibling and the sibling's name vanishes. `registered_names_are_unique` catches it; `every_bound_command_name_is_registered` catches the keymap side, and checks **both** halves of `dispatch`'s `registry.interactive(name, args, ctx).ok()?` — the name resolves *and* the args parse — because improvise-s13 moved `enter-mode ["export-prompt"]`/`["formula-edit"]` out of the nullary class, where a typo'd mode name compiles into a dead key. It builds the `CmdContext` in each keymap's own `AppMode`, since `commit-buffer item` reads `ctx.mode`.
|
||||
16. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic. Its counterpart on the effect side is `simple_effect!` in `ui/effect.rs`, which declares a one-field newtype that writes one place on the `App`. Its **kind argument is mandatory** — `simple_effect!(Name(T) => |app| place, EffectKind::ViewOnly)` — even though every call site so far answers `ViewOnly`: hard-coding it reintroduced exactly the default gotcha 3 does without, since `$place` is an unconstrained expression and a one-line `active_view_mut().number_format` setter would have reported `ViewOnly` with the suite green (improvise-8fy). Exactly six wrappers return `unless_unknown(needs, effect)` rather than the bare effect (gotcha 51): `set-axis`, `set-page`, `toggle-group`, `hide-item`, `show-item` and `switch-view`. The rest return bare effects — `delete-view` (whose `DeleteView` effect refuses an unknown view itself), `add-item`, `add-items`, `set-cell`, `create-view` — and `remove-formula` checks in `execute`. Likewise, mode entry has no per-mode shim commands — bind `enter-mode <mode-name>` (improvise-s13); the name is decoded by `parse_mode_name` in `registry.rs`, and `EnterMode` clears the mode's minibuffer on the way in.
|
||||
17. **Float equality inside formula eval has one epsilon, and the div/0 guard shares it.** `FLOAT_EQ_EPSILON = 1e-10` in `model/types.rs` is used by `=`/`!=` **and** by the division-by-zero guard (`rv.abs() < FLOAT_EQ_EPSILON`), in both `eval_expr` and its aggregated twin — six uses across the two evaluators (a div/0 guard plus the `Eq` and `Ne` arms in each), one constant. This gotcha previously recorded the opposite (`=`/`!=` epsilon, div/0 exact) and was stale; the mix is gone. Why an epsilon at all and why this one: `context/code/core.md` §4.
|
||||
18. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this — and so, since improvise-shyt, do `DataStore::matching_cells`/`matching_values`: the secondary index **narrows** the candidates, then `matching_positions` resolves each to its position in the primary `IndexMap`, sorts, and reads back. **An index whose values are `HashSet`s is for O(1) lookup, never for iteration.** Walking it gave a different order in every process, which was not tidiness. Records-mode rows reordered between sessions; `SUM`/`AVG` drifted in the last bits because `f64` addition is not associative (`Min`/`Max`/`Count` and the text-unanimity rule of gotcha 7 are order-free, so they were never affected); and worst, the **drill snapshot** froze its record list in hash order while `drill_state.pending_edits` are keyed by *row index* (gotcha 1), so the same drill opened in a second process addressed **different records** — silent misdirected writes, and nothing in the tracker knew. Measured at 18,000 cells: +20% worst case on `matching_values`, and `matching_cells` 7% *faster*, sorted positions resolving `CellKey`s with better locality. `DataStore::live_records` was the open choice and is now decided (improvise-2yc2): it walks `cells` and projects insertion order onto records, deliberately paying O(cells) rather than O(records), because `cells`' order is already the answer `matching_positions` gives and a second answer to "what order is this store in" is the disease. It is derived per call rather than memoised on `RecordEntry`, since a stored position is a copy and a record deleted and re-created would hold its old one. Note what is *not* on offer: an unordered return — every iterator has an order, and a type that merely disclaims one still hands the caller hash order, which is the guard living one level away from the hazard and the shape this whole class of bug returns through. **The second real instance was `import::analyzer::analyze_records`** (improvise-7yx), which built its distinct values through a `HashSet`: `build_model` adds items in exactly that order and `IndexMap` preserves it, so the *same CSV imported twice* produced two different `.improv` files and two different row orders. Now first-seen order, pinned by a five-iteration test because each `HashSet` gets its own `RandomState`. Also answered here since: an **empty `partial`** matches every cell vacuously (improvise-hkt6) — it used to index `sets[0]` and panic, unreachable only because both callers guarded. **A group's fold reads through the same rule.** `Model::fold_stored` folds a collapsed group's stored values once, over every record the group covers, through `DataStore::spanned_values` → `spanned_positions` (improvise-yb4b) — one scan, not a lookup per member. `spanned_positions` chooses **by index-set size** between narrowing once by the base key and narrowing once per covered item of the smallest span, whichever visits fewer (improvise-jq0m), and **both arms return store order** — sorted positions, as `matching_positions` does. That is load-bearing, not tidy: a fold that returned hash or per-item order would reintroduce the `SUM` drift above, and the choice between the arms would then be visible in the last bits of a total. **A partial as wide as the widest stored cell needs no narrowing** (improvise-felo.13): `DataStore` keeps `arities` (how many stored cells have each width), and `exact_positions` answers a strictly-sorted partial at least that wide with one probe of `cells`, since only an equal cell can match; anything else falls through to the index. The answer is the same sorted positions.
|
||||
19. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior. **Which cells refuse an editor is one function, asked at both ends of an edit**: `cmd::core::uneditable(model, layout, row, col)` is the shared decline for **opening** an editor (`EnterEditAtCursor`) and for **committing** one (`CmdContext::committable_cell_key`) — a fold or other aggregate, a formula's result, or a `CellId::NonItem` (improvise-enz6, improvise-gk51). It reads the cell's `CellId` through `project::cell_id`, the same derivation the projection uses, and the browser's `resolve::declined_at` matches on the projected `CellId`, so the two front ends refuse the same cells. Asking only at open is the bug enz6 was: Enter-to-advance moved onto a collapsed group's row and committed a write to the group coordinate. **`CellId::NonItem { category, item }` is a stored value whose coordinate names no item of its category** — records mode can draw one, and no write can address it, so it is projected as its own variant rather than as a `Record` that the host then refuses (gk51 was exactly that mismatch). Its one wording is `improvise_protocol::non_item_refusal`.
|
||||
@@ -425,14 +425,14 @@ improvise-gp/src/fit.rs 1778 / 35t one fit end to end: a View's records in,
|
||||
cmd/core.rs 475 / 2t Cmd trait, CmdContext, CmdRegistry, parse helpers
|
||||
cmd/registry.rs 903 / 13t default_registry() — all registrations
|
||||
cmd/navigation.rs 507 / 12t Move, EnterAdvance, Page*
|
||||
cmd/cell.rs 724 / 20t ClearCell, YankCell, PasteCell, TransposeAxes, SaveCmd
|
||||
cmd/cell.rs 724 / 20t ClearCellCommand, YankCell, PasteCell, TransposeAxes, SaveCmd
|
||||
cmd/commit.rs 1304 / 27t CommitAndAdvance, CommitBuffer(CommitKind), CommitExport,
|
||||
CommitSave
|
||||
cmd/effect_cmds.rs 519 / 6t effect_cmd! macro, 25+ simple wrappers, `unless_unknown` (gotcha 51)
|
||||
cmd/fit.rs 293 / 8t fit-measure: evolve a tree over the active view and store it
|
||||
as an ordinary `## Formulas` line — nothing GP-specific is
|
||||
left in the document afterwards
|
||||
cmd/grid.rs 988 / 22t ToggleGroup, ViewNavigate, DrillIntoCell, TogglePruneEmpty
|
||||
cmd/grid.rs 988 / 22t ToggleGroupAtCursor, ViewNavigate, DrillIntoCell, TogglePruneEmpty
|
||||
cmd/mode.rs 469 / 13t EnterMode, Quit, EditOrDrill, EnterTileSelect
|
||||
cmd/panel.rs 936 / 24t Panel toggle/cycle/cursor, formula/category/view panels
|
||||
cmd/search.rs 202 / 4t SearchNavigate, SearchOrCategoryAdd, ExitSearchMode
|
||||
|
||||
@@ -454,7 +454,6 @@ work_done('improvise-31oe.3',bugfix,'PR #316, merge 66931785. The command langua
|
||||
work_done('improvise-31oe.2',bugfix,'PR #316, merge 66931785. set-axis, set-page, toggle-group, hide-item, show-item and switch-view check at APPLY time, through the ViewOnly off-wire effect RequireKnown(Needs) built by effect_cmds::unless_unknown, that the category, item, group or view exists, and refuse and abort the batch otherwise; a refused name stops the rest of a : line, while headless improvise cmd runs each segment as its own batch (improvise-31oe.14, open). context/code/cli.md section 15.').
|
||||
work_done('improvise-8jk6.3',feature,'PR #322, merge d571abbd. A date column imports as ONE hierarchical category (Year > Quarter > Month) by default and opens folded to its years; --split-date <field> and s on the wizard date step restore the flat split (DateShape::Split); --split-date naming a non-date field and --axis naming a category the import did not build are refused with exit 1. context/code/formula-and-io.md, Import section.').
|
||||
work_done('improvise-d0n6',bugfix,'PR #322, merge d571abbd. Plan::spelling_clash refuses a raw unparsed date spelled like a generated month, quarter or year item, because the two would silently merge; asked at the wizard ConfigureDates step and again at build time so the headless import cannot reach it.').
|
||||
work_done('improvise-8jk6.6',feature,'PR #321, merge 7e879702. headless import --strict: with --no-wizard any skipped record is a refusal before the build and nothing is written; without it a partial skip writes the model and warns with a count. A folder whose CSVs are all header-only is refused as an import that kept no record. context/code/cli.md section 13.').
|
||||
work_done('improvise-8jk6.6',bugfix,'PR #321, merge 7e879702. headless import --strict: with --no-wizard any skipped record is a refusal before the build and nothing is written; without it a partial skip writes the model and warns with a count. A folder whose CSVs are all header-only is refused as an import that kept no record. context/code/cli.md section 13.').
|
||||
work_followup('improvise-8tk','improvise-lae','no-file-path UX: prompt for a path rather than only erroring. ExportPrompt is CSV-only, so a new SavePrompt TextEntry mode committing to SaveAs is the likely shape.').
|
||||
work_followup('improvise-6sq','improvise-ct5','i on an empty records grid reports \'cannot drill\', leaking a pivot-mode concept into records mode').
|
||||
|
||||
Reference in New Issue
Block a user