Merge pull request #9: wave-1 review fixes
fix: wave-1 review findings — a ghost formula-target column, EffectKind classification, the protocol fold contract, and vacuous tests
This commit is contained in:
Ed Langley
GitHub
+10
-1
@@ -15,6 +15,8 @@
|
||||
{"_type":"issue","id":"improvise-km8","title":"1.2 Fix Cargo.toml metadata","description":"Add missing package fields: description, repository, homepage, documentation, readme, keywords, categories, license. Description: 'Terminal pivot-table modeling in the spirit of Lotus Improv'. Keywords: tui, pivot, spreadsheet, data, improv. Categories: command-line-utilities, visualization.","status":"closed","priority":1,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:06:48Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:20:17Z","close_reason":"Metadata already present: description, license, repository, homepage, readme, keywords, categories all populated.","dependency_count":0,"dependent_count":2,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-902","title":"1.1 Audit and remove or update context/SPEC.md","description":"Compare context/SPEC.md against actual code. If spec contradicts code significantly, delete it. Optionally move salvageable conceptual content to docs/design-notes.md with a staleness disclaimer. Do not try to bring spec into sync.","status":"closed","priority":1,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:06:46Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:10:05Z","close_reason":"Audited SPEC.md against code. Found minor inaccuracies (storage internals, wizard step count, search mode representation) but no major architectural contradictions. Deleted SPEC.md since it was largely redundant with repo-map.md and design-principles.md. Salvaged product vision and non-goals into docs/design-notes.md with staleness disclaimer.","dependency_count":0,"dependent_count":1,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-tik","title":"Phase 1: Repository hygiene","description":"Raise the quality floor of what a stranger sees when they land on the repo. Subtasks cover spec audit, Cargo.toml metadata, publish-readiness, CSV audit, and example files.","status":"closed","priority":1,"issue_type":"feature","owner":"el-github@elangley.org","created_at":"2026-04-09T04:05:45Z","created_by":"Edward Langley","updated_at":"2026-04-09T07:22:34Z","closed_at":"2026-04-09T07:22:34Z","close_reason":"All 5 subtasks complete: SPEC.md audited, Cargo.toml metadata fixed, publish dry-run clean, CSV audit done, example files created.","dependencies":[{"issue_id":"improvise-tik","depends_on_id":"improvise-2fr","type":"blocks","created_at":"2026-04-08T23:37:31Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-902","type":"blocks","created_at":"2026-04-08T23:37:30Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-bv1","type":"blocks","created_at":"2026-04-08T23:37:32Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-km8","type":"blocks","created_at":"2026-04-08T23:37:30Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-n1h","type":"blocks","created_at":"2026-04-08T23:37:32Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":5,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-cu7s","title":"[bug] Formula target keeps its pipe delimiters, so a quoted target lands on the wrong _Measure item","description":"From the wave-1 gp-tree review (2026-09-19). CONFIRMED by probe. Likely PRE-EXISTING and user-facing, not introduced by the gp work.\n\nimprovise-formula/src/parser.rs:55 does target = Some(inner.as_str().trim().to_string()), taking the RAW span; formula.pest:20 documents this deliberately ('The target keeps its raw text (including pipes, if any)'). So parsing '|Total Revenue| = ...' yields Formula { target: \"|Total Revenue|\" } -- 15 characters including the pipes -- whereas an Expr::Ref for the same name is correctly unquoted.\n\nConsequence: Model::add_formula then targets a _Measure item whose literal name contains pipes, which never matches the user's item 'Total Revenue', and on save the writer quotes it a SECOND time. Probe confirmed targets 'Total Revenue', 'SUM', 'a|b' and '' all come back pipe-wrapped.\n\nINVESTIGATE FIRST -- the reviewer found this via improvise-gp, but the same parse path serves the formula minibuffer and the .improv '## Formulas' load path. Check whether a user typing a pipe-quoted target in the TUI, and a .improv file containing one, hit the same bug. If so this is a user-facing data bug, not a gp-only one, and its priority should rise.\n\nFIX LOCATION IS A DECISION: unquote in improvise-formula so target matches Expr::Ref's treatment (consistent, but changes a documented grammar behaviour and anything relying on the raw span), or unquote at each call site. Whichever, improvise-gp must stop asserting the broken shape: lib.rs:267-276 an_awkward_target_name_is_quoted_and_round_trips asserts parsed.target == \"|Total Revenue|\" and calls it correct.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:43:11Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:43:11Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-35jb","title":"every_bound_command_name_is_registered checks the name but never that the args parse","description":"From the wave-1 review (2026-09-19). The most useful finding of the review.\n\nkeymap.rs's every_bound_command_name_is_registered walks every binding and checks the command NAME is in registry.names(). It never checks that the bound ARGS parse. But Keymap::dispatch does registry.interactive(name, args, ctx).ok()? -- so a binding whose args are unparseable is ALSO a silently dead key, which is the exact failure mode the test was written to prevent.\n\nimprovise-s13 made this live: it moved two bindings out of the covered class (nullary) into the uncovered one (arg-carrying) -- enter-mode [\"export-prompt\"] and enter-mode [\"formula-edit\"]. A typo such as \"export_prompt\" would compile, pass the whole suite, and produce a dead Ctrl-e. The reviewer verified by hand that both resolve TODAY, so there is no live defect -- the check is what is missing.\n\nFIX: in that same walk, build a CmdContext and assert ks.registry().interactive(name, args, \u0026ctx).is_ok() rather than only checking membership in names(). Per doctrine(check_the_checker), prove it fires by injecting a typo'd arg before keeping it.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:42:36Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:42:36Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-hzoh","title":"simple_effect! reintroduces the EffectKind default that gotcha 3 exists to prevent","description":"From the wave-1 code review (2026-09-19), CONFIRMED by probe.\n\nsimple_effect! hard-codes EffectKind::ViewOnly and takes an unconstrained $place:expr. This probe compiles, is a textbook LayoutGeometry change (SetNumberFormat right next door declares exactly that), reports ViewOnly, and the full improvise-command suite passes 294/0 with it present:\n\n simple_effect!(AbusiveSetNumberFormat(String) =\u003e |app| app.model_state.workbook.active_view_mut().number_format);\n\ninvariant(effect_kind_no_default) exists because a mis-declared kind is SILENT -- ViewOnly never reaches other clients. kind() having no default forces a declaration; the macro reintroduces the default under one name, for a category defined by SYNTAX ('one field, one place') rather than SEMANTICS ('per-client'). The doc comment states the intended boundary honestly, but a comment is not a constraint. The only pin, view_only_effects_leave_shared_state_alone, is a hand-maintained Vec that nothing forces you to extend -- and SetYanked, one of the eight macro'd effects, is already missing from it.\n\nThe eight effects currently declared through the macro were each checked and ARE genuinely ViewOnly, so there is no live defect -- this is about the shape.\n\nFIX (cheapest that keeps the macro): give it a mandatory kind argument, e.g. simple_effect!(Name(T) =\u003e |app| place, ViewOnly), so the classification is written per call site even when the answer is always the same -- the same reasoning that gave kind() no default. Also add SetYanked to the pin list.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:40:49Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:40:49Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-2v7t","title":"Protocol fold contract: headline tests are vacuous, ordering contract unenforced","description":"From the wave-1 code review (2026-09-19), CONFIRMED by fault injection.\n\n(A1) splitting_a_stream_anywhere_gives_the_same_cache and a_snapshot_seed_is_just_the_head_of_the_stream both assert fold(prefix)+fold(rest) == fold(whole). Since folded()'s only body IS default() + a loop of apply(), that identity is the DEFINITION of a left fold and holds for any apply() whatsoever. Proof: replacing GridCache::apply's cells.get_mut(row).get_mut(col) with get_mut(0).get_mut(0) -- every cell written to (0,0) -- left BOTH tests passing while the two real tests failed.\nThe property the architecture rests on is ORDER SENSITIVITY: a Cells update arriving before its framing RowLabels/ColLabels is silently dropped, so the fold is not commutative and nothing tests reordering. Also, the snapshot test uses the literal prefix stream[..2] as the seed; a real server sends a COMPACTED seed (current labels + cells), and that compaction-vs-replay equivalence -- what 'seeded then follows the stream' means in production -- is untested.\n\n(A2) A shape change without Invalidated tears the grid. fit_to_labels truncates/extends by POSITION, so removing a middle column leaves surviving cells under the wrong labels: ColLabels[A,B,C]+Cells, then ColLabels[A,C] =\u003e the column labelled C shows B's value. GridUpdate::Invalidated exists to prevent this and its doc states the ordering contract, but the contract is PROSE ONLY -- nothing in the types requires it, and shrinking_the_labels_drops_the_cells_that_no_longer_fit blesses a bare RowLabels shrink (the one case where preserve-by-position is right). There is no producer anywhere in the tree, so the first from_app author has only a doc comment to go on.\n\n(A3) col_widths is never re-fitted: ColLabels[A]+ColWidths[8] then ColLabels[A,B] leaves labels len 2, widths len 1. Cache stays total, but a renderer zipping widths with labels indexes out of range.\n\n(A5) populate_before_roundtrip regression: every_command_round_trips_through_json previously covered CellsUpdated with a populated CellDisplay, and now covers only Command::Project(Projection::Grid(GridUpdate::Invalidated)) -- the ONE variant in the new vocabulary carrying no payload. Also CellDisplay::numeric (the field deciding alignment) is never round-tripped at a non-default value, and Snapshot is never round-tripped at all and derives neither Clone nor PartialEq.\n\nFIX: make the tests prove order sensitivity and compaction-equivalence rather than the fold axiom; enforce or type-encode the Invalidated ordering contract; re-fit col_widths; restore populated round-trips.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:40:48Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:40:48Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-l3at","title":"AddCategory declares ModelChange::CellData but changes grid geometry","description":"Found while fixing improvise-djn (2026-09-19). The AddCategory effect declares EffectKind::Model(ModelChange::CellData), but adding a category changes grid GEOMETRY, not cell data.\n\nPer repo-map gotcha 3 this class of mistake is silent: EffectKind drives what reaches other clients, there is no default, and a wrong classification produces no compile error and no test failure today -- it only shows up as a connected client whose layout never updates. That makes it exactly the kind of defect the 'no default kind' rule exists to catch, and it slipped through anyway because the rule only forces you to WRITE a kind, not to write the right one.\n\nCheck the sibling effects at the same time (AddItem, RemoveItem, RemoveCategory, anything else touching category membership) -- if one is wrong the others likely are too.\n\nSuggested test: assert the declared kind for each membership-changing effect, so the classification is pinned rather than assumed.","notes":"\n--- CONFIRMED by the wave-1 review (2026-09-19), and it is TWO effects, not one ---\nAddCategory (effect.rs:96-98) AND RemoveCategory (effect.rs:314-316) both\ndeclare Model(ModelChange::CellData). Both go through Workbook, which mutates\nEVERY view's axis map:\n workbook.rs:59-61 add_category -\u003e for view in views { view.on_category_added(\u0026name) }\n workbook.rs:80-82 remove_category -\u003e view.on_category_removed(name)\nThat is view_config_fingerprint's territory and matches LayoutGeometry's own\ndoc ('Axis assignment ... the grid re-lays out'), not CellData's. Probe\nconfirmed each changes serde_json::to_string(\u0026workbook.views).\n\nSIBLINGS CHECKED AND CORRECT -- do not 'fix' these: AddItem, AddItemInGroup,\nRemoveItem go through model only and leave view config untouched; SetCell,\nClearCell, SortData, AddFormula, RemoveFormula likewise. SetAxis,\nTransposeAxes, CreateView, SetNumberFormat, TogglePruneEmpty already declare\nLayoutGeometry correctly. The defect is confined to the two CATEGORY effects.\n\nFailure scenario once a projector exists: a client told CellData emits\nGridUpdate::Cells but no Invalidated + fresh label pair, so a new category's\nrow never appears, or a removed category leaves a column whose label is gone\nfrom every view.\n\nLATENT TODAY: kind() has no production caller yet -- it is read only by\neffect.rs's classification tests. That is the argument for fixing now, while\nthere is no behaviour to regress.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:36:44Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:41:01Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
@@ -105,6 +107,11 @@
|
||||
{"_type":"issue","id":"improvise-pby","title":"2.2 Write the README","description":"Replace existing README.md with new one under 250 lines. Sections in order: Title, one-sentence pitch, inline demo GIF, why this exists (Lotus Improv reference), quick start, key bindings, installation (Nix/crates.io/prebuilt - no Homebrew), codebase overview, expectations disclaimer, license. No badges, TOC, or contributing guide.","status":"closed","priority":2,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:07:32Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:02:52Z","close_reason":"README written with all 10 required sections, committed. License updated to Apache-2.0 per user request.","dependency_count":0,"dependent_count":2,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-ihv","title":"2.1 Add Nix tooling for asciinema, VHS, and cargo-dist","description":"Modify flake.nix to add pkgs.asciinema and pkgs.vhs to dev shell nativeBuildInputs. Add cargo-dist if packaged in nixpkgs. Add runtime deps for VHS (ttyd, ffmpeg) if needed. Verify tools work via nix develop --command. Add nix run app or shell script for demo recording workflow.","status":"closed","priority":2,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:07:30Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:30:19Z","close_reason":"asciinema, vhs, cargo-dist added to flake.nix dev shell and verified. cargo-dist wrapper enables 'cargo dist' subcommand. scripts/record-demo.sh added.","dependency_count":0,"dependent_count":3,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-yk4","title":"Phase 2: README and demo artifacts","description":"The main launch work. README is 80% of the launch, demo artifacts are 20%. Covers Nix tooling for asciinema/VHS, writing the README, creating demo GIF and asciinema casts, and verifying all artifacts.","status":"closed","priority":2,"issue_type":"feature","assignee":"spot","owner":"el-github@elangley.org","created_at":"2026-04-09T04:05:49Z","created_by":"Edward Langley","updated_at":"2026-04-14T07:47:43Z","closed_at":"2026-04-14T07:47:43Z","close_reason":"All 5 subtasks closed; artifacts verified present and tracked in git (README.md, docs/demo.gif, docs/demo.tape, docs/casts/*.cast).","dependencies":[{"issue_id":"improvise-yk4","depends_on_id":"improvise-abz","type":"blocks","created_at":"2026-04-08T23:37:37Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-d4w","type":"blocks","created_at":"2026-04-08T23:37:36Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-ihv","type":"blocks","created_at":"2026-04-08T23:37:34Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-odx","type":"blocks","created_at":"2026-04-08T23:37:35Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-pby","type":"blocks","created_at":"2026-04-08T23:37:36Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":5,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-i2m5","title":"improvise-io still carries its own pipe-quoting copy, without the keyword guard","description":"Follow-up from the wave-1 review fixes (2026-09-19). Pipe quoting now has one home -- improvise_formula::quote (quote_name/is_bare_name) -- and improvise-gp re-exports it. improvise-io/src/persistence/quoting.rs still has its own pub(super) is_bare_name/escape_pipe/quote_name/pipe_quote, and that copy lacks the keyword guard. It also holds a second copy of the UNQUOTING rule (unescape_pipe vs parser.rs::unquote_pipe).\n\nSwitching io to improvise_formula::quote_name is not purely mechanical: it would change .improv output for a category or item literally named SUM/WHERE/etc to |SUM|. That still parses (improv.pest accepts pipe_quoted), and the persistence round-trip tests will show the delta -- so the change is safe but visible, and someone should look at the diff rather than assume.\n\nDecide the keyword guard deliberately. It is NOT a correctness fix: probing showed the grammar accepts bare keywords in every position ('WHERE = A', 'X = SUM', 'SUM(WHERE)' all parse). It was kept in improvise-formula because that acceptance is an emergent property of PEG ordering rather than a stated rule, and a pipe-quoted name is an identifier everywhere with nothing to fall through. Cost is cosmetic.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T20:50:39Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:50:39Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-ipgl","title":"The pipe-quoting rule now has three copies, and two of them already differ","description":"From the wave-1 gp-tree review (2026-09-19). design-principles.md §3 calls this convention 'shared between the .improv persistence format and the formula parser's identifier syntax' -- one rule. It is now spelled three times:\n * improvise-io/src/persistence/quoting.rs:4-56 (is_bare_name/escape_pipe/quote_name), pub(super)\n * improvise-formula's tokenizer\n * improvise-gp/src/lib.rs:86-112 (new)\nThe gp copy was FORCED, not lazy -- the io one is pub(super) and genuinely unreachable. But the copies already diverge: gp guards the seven keywords (WHERE/SUM/AVG/MIN/MAX/COUNT/IF), io does not.\n\nNatural single home is improvise-formula, which both improvise-io and improvise-gp already depend on. Moving it there also settles which behaviour is canonical -- decide the keyword guard deliberately rather than by which copy you happened to read.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T20:43:13Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:43:13Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-8x9o","title":"Vendored SplitMix64 has no known-answer test","description":"From the wave-1 gp-tree review (2026-09-19). CONFIRMED by fault injection: changing the generator's gamma constant from 0x9E3779B97F4A7C15 to ...7C17 left 27/27 tests passing.\n\nimprovise-lzn's whole promise (gp-tree/src/lib.rs:58-68) is that 'a bug report must be reconstructible from its seed alone'. Nothing in the workspace pins the generator's output, so a future cleanup of the vendored RNG silently invalidates every seed already printed by a fit and every Model.seed already saved -- with no test failing. This is doctrine(check_the_checker) exactly: the guarantee nobody has seen fail.\n\nFIX: three lines -- a known-answer vector, e.g. assert SplitMix64::new(0).range(0, 1_000_000) equals its current value, and one more for a non-zero seed.\n\nRelated, LOW: the determinism pair (the_same_seed_fits_the_same_formula / a_different_seed_fits_a_different_formula) does not by itself prove the search MOVED -- a GA whose breed did nothing would still differ across seeds because the initial random_tree differs. It happens to be true (instrumented: best cost 3.55 -\u003e 1.80 and 6.46 -\u003e 0.76 over 20 generations), but assert it: compare the fit against the generation-0 best.","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:43:12Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:43:12Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-ki7c","title":"EnterMode writes a runtime-named buffer without going through write_buffer","description":"From the wave-1 review (2026-09-19). Latent, not live.\n\nimprovise-gdl made cmd::core::write_buffer(name, value) the single accessor for a RUNTIME-NAMED buffer, so that the 'search' name routes to SetSearchQuery instead of a dead map entry. But cmd/mode.rs's EnterMode still builds effect::SetBuffer { name: mb.buffer_key.to_string(), .. } directly. Every other remaining direct SetBuffer uses a LITERAL name ('command', 'export', 'edit') that is statically not 'search', so EnterMode is the one site that writes a name it does not know at compile time and bypasses the router.\n\nUnreachable today because no AppMode carries buffer_key: \"search\". Concrete failure when that changes: the natural follow-up to 'SearchMode is Normal + a flag' (gotcha 11/14) is a real AppMode::Search { minibuf: ... buffer_key: \"search\" }. EnterMode would then clear buffers[\"search\"] while append-char/pop-char read and write view_state.search_query -- so entering search mode would leave the PREVIOUS query on screen. Exactly the desync gdl closed, reopened from the other side.\n\nFIX: one line -- effects.push(write_buffer(\u0026mb.buffer_key, String::new())) in EnterMode.","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:42:38Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:42:38Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-odi4","title":"Nothing ties AppMode::hint_text to the actual keymap","description":"From the wave-1 review (2026-09-19). only_quit_has_no_hint asserts a hint is PRESENT; nothing asserts it is ACCURATE. Rebinding a mode's keys leaves its hint silently lying to the user -- the same class of silent drift that doctrine(binding_dump_diff) exists for.\n\nConcretely: improvise-1c9 added 'Enter:export Esc:cancel — type a file path' to ExportPrompt. The reviewer confirmed it matches the real keymap today (Enter =\u003e SEQ[commit-export, clear-buffer export], Esc =\u003e SEQ[clear-buffer export, enter-mode normal]). Nothing keeps that true.\n\nFIX: for each mode, split its hint on whitespace and for every 'Key:verb' token assert keymap.lookup(key, NONE).is_some() in that mode. Prove it fires by rebinding or removing a key named in a hint.\n\nCosmetic while there: every other hint uses two spaces around the em dash; the new ExportPrompt one uses one.","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:42:37Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:42:37Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-ksst","title":"A gp-tree fit records its seed but not its GA config","description":"Follow-up from improvise-lzn (2026-09-19). data::Model now records the seed, so a fit looks reproducible, but the GA configuration (population, generations, elite count, penalty) is not recorded with it. A saved model is therefore only reproducible if the caller independently remembers the config -- and nothing says so, which is the trap: the seed's presence implies a completeness that is not there.\n\nEither record the config alongside the seed in data::Model, or document at the seed field that it is necessary but not sufficient.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T20:38:55Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:38:55Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-szrt","title":"Wire improvise-gp into improvise (nothing depends on it yet)","description":"Follow-up from improvise-izk (2026-09-19). crates/improvise-gp exists and is tested but has no consumers -- it is a bridge with nothing crossing it. The next step is a user-facing path: fit a tree from the current model/view and install the emitted formula, presumably as a command in the Cmd/Effect layer.\n\nDesign constraints when doing it: the Cmd must stay pure and return effects (the fit itself is expensive, so decide whether it belongs in an Effect or off the main loop), and the emitted formula must go through the same add_formula path as a hand-typed one so _Measure target handling stays identical.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T20:38:55Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:38:55Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-d44s","title":"gp_tree::data::preprocess writes to stderr from library code","description":"Follow-up from improvise-0yr (2026-09-19). preprocess() eprintln!s '(column X is entirely missing — skipped)'. That was correct in examples/fit_csv.rs but it is now library code, and a library deciding to write to the user's stderr is a layering problem -- the caller cannot suppress, redirect or localize it.\n\nLeft in place deliberately during 0yr so the extraction stayed byte-identical (the baselines it was verified against include that output). Fix by returning the skipped columns as data -- a warnings vec on Dataset, mirroring how improvise-io's build_model_with_warnings already surfaces import warnings rather than printing them.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T20:38:54Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:38:54Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
@@ -134,7 +141,7 @@
|
||||
{"_type":"issue","id":"improvise-gai","title":"AGENTS.md session-completion workflow has bare 'git merge' with no target","description":"AGENTS.md Session Completion step 4 reads 'git merge' with no ref (CLAUDE.md's copy correctly says 'git merge origin/main'). An agent following AGENTS.md literally runs bare git merge, which errors or merges the wrong thing. The two files also duplicate the whole beads/session block with this one divergence — consider a single source of truth. Found in systematic review 2026-06-09.","status":"closed","priority":3,"issue_type":"bug","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-06-10T03:59:03Z","created_by":"Edward Langley","updated_at":"2026-06-10T04:46:48Z","closed_at":"2026-06-10T04:46:48Z","close_reason":"Fixed (test-first where applicable): records_display returns None for OOB columns (f04fe51); try_axis_of added + axis_of invariant documented + cycle_axis no longer panics on unregistered categories (f04fe51); scrolled header suppression fixed via pure show_sublabel helper for both row and column headers (df9a02b); CyclePanelFocus cycles from current panel with wrap (9e02245); AGENTS.md merge target corrected (9634c95).","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-wg1","title":"Add GNU Global tags and graphviz artifacts to .gitignore","description":"git status shows untracked GPATH, GRTAGS, GTAGS (GNU Global, generated by the gtags devShell tooling added in 62d18ff) and graph.dot/graph.html/graph.svg at the repo root. None are covered by .gitignore, risking accidental commits. Add: GPATH, GRTAGS, GTAGS, /graph.dot, /graph.html, /graph.svg (or /graph.*). Found in systematic review 2026-06-09.","notes":"2026-06-09: the git-smart-commit hook added these artifacts to .git/info/exclude (machine-local), so they no longer show in status here — but the shared .gitignore still lacks them; issue remains valid for other clones.","status":"closed","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-06-10T03:59:00Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:32:19Z","closed_at":"2026-09-19T20:32:19Z","close_reason":"Added exact-name ignores to the root .gitignore: GTAGS/GRTAGS/GPATH/GSYMS for GNU Global, and root-anchored /graph.dot, /graph.html, /graph.svg for the graphviz artifacts. Deliberately not globs like *.svg, which would swallow checked-in assets. Verified no tracked file becomes ignored: 'git ls-files | git check-ignore --stdin' printed nothing.","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-v9w","title":"Remove unused pest/pest_derive from root crate [dependencies]","description":"Root Cargo.toml:38-39 lists pest and pest_derive under [dependencies], but nothing under src/ uses them (grammar parsing lives in improvise-io; examples use pest_meta from dev-dependencies). They are also duplicated under [dev-dependencies]. Remove from [dependencies]; verify with cargo build --workspace \u0026\u0026 cargo test --workspace. Found in systematic review 2026-06-09.","notes":"Also unused: dirs = \"6\" (root Cargo.toml:36). Found while doing improvise-6mq, whose acceptance criterion 3 was 'dirs crate usage is isolated behind a function that can be stubbed for wasm' — there is no dirs usage anywhere under src/ or crates/*/src/ to isolate, so that criterion is vacuous. Same class as the pest/pest_derive removal this ticket already covers; fold it in.\nGROOMED 2026-09-19: still true; line numbers shifted — pest/pest_derive are Cargo.toml:40-41 under [dependencies] and 44-46 under [dev-dependencies] (with pest_meta).","status":"closed","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-06-10T03:58:53Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:32:18Z","closed_at":"2026-09-19T20:32:18Z","close_reason":"Removed pest, pest_derive and dirs from the root crate [dependencies]. Premise verified by build, not by reading: examples/gen-grammar.rs uses pest_meta but gets it from [dev-dependencies], so cargo build --examples stays green. dirs had no usage anywhere under src/ or crates/*/src/ and was folded in per this ticket's own notes. cargo build --workspace, cargo test --workspace (801 passed), cargo build --examples and the four-crate wasm32 check all clean. Cargo.lock drops only dirs + transitives; pest* remain for improvise-formula/improvise-io.","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-gdl","title":"SetBuffer effect special-cases the \"search\" buffer name","description":"crates/improvise-command/src/ui/effect.rs:433-440: SetBuffer routes to view_state.search_query when self.name == \"search\", otherwise to the buffers map. Callers must know the magic name; the special case is undiscoverable and weakens the buffer abstraction. Fix: a dedicated SetSearchQuery effect (or fold search_query into the buffers map). Related to improvise-k8h (minibuffer buffer_key strings). Found in systematic review 2026-06-09.","notes":"STALE REF (groomed 2026-08-30, line numbers shifted by this session's work): effect.rs:433-440 -\u003e SetBuffer is now at crates/improvise-command/src/ui/effect.rs:614 (pub struct SetBuffer). The special case is unchanged: SetBuffer still routes name==\"search\" to view_state.search_query.\nGROOMED 2026-09-19: still true; SetBuffer is effect.rs:621-630 now, not 433-440. The comment on the special case still reads \"backward compat\".","status":"closed","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-06-10T03:58:41Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:35:54Z","closed_at":"2026-09-19T20:35:54Z","close_reason":"Premise true and worse than stated: the 'search' special case was in three places, not one (the SetBuffer effect plus AppendChar and PopChar). Fixed with one cmd::core::write_buffer(name, value) placed under the read_buffer it mirrors; SetBuffer is now a plain map write. Side effect: 'clear-buffer search' now clears the query instead of parking an unread map entry -- it previously worked only by accident of the effect's special case, and no keymap binds it, so no user-visible change.","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-gdl","title":"SetBuffer effect special-cases the \"search\" buffer name","description":"crates/improvise-command/src/ui/effect.rs:433-440: SetBuffer routes to view_state.search_query when self.name == \"search\", otherwise to the buffers map. Callers must know the magic name; the special case is undiscoverable and weakens the buffer abstraction. Fix: a dedicated SetSearchQuery effect (or fold search_query into the buffers map). Related to improvise-k8h (minibuffer buffer_key strings). Found in systematic review 2026-06-09.","notes":"STALE REF (groomed 2026-08-30, line numbers shifted by this session's work): effect.rs:433-440 -\u003e SetBuffer is now at crates/improvise-command/src/ui/effect.rs:614 (pub struct SetBuffer). The special case is unchanged: SetBuffer still routes name==\"search\" to view_state.search_query.\nGROOMED 2026-09-19: still true; SetBuffer is effect.rs:621-630 now, not 433-440. The comment on the special case still reads \"backward compat\".\n\n--- CORRECTION to this ticket's close reason (2026-09-19 review) ---\nThe close reason said 'no keymap binds it, so no user-visible change'. That is\ntrue only of KEYBINDINGS, and was verified: zero occurrences of\nclear-buffer [\"search\"] across all 9,760 (mode, key, modifier) lookups in all\n16 modes. But ClearBufferCmd is registered with a free-form arg, so\n':clear-buffer search' typed in CommandMode, a script file, and\n'improvise cmd clear-buffer search' all reach it.\n\nSo the behaviour delta is reachable by the user, just not by a keypress: it\nused to park a dead map entry and now blanks view_state.search_query\n(SetSearchQuery is a bare ViewOnly setter with no side effects). Harmless and\narguably more correct -- but do NOT record 'unreachable' as an invariant.","status":"closed","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-06-10T03:58:41Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:42:43Z","closed_at":"2026-09-19T20:35:54Z","close_reason":"Premise true and worse than stated: the 'search' special case was in three places, not one (the SetBuffer effect plus AppendChar and PopChar). Fixed with one cmd::core::write_buffer(name, value) placed under the read_buffer it mirrors; SetBuffer is now a plain map write. Side effect: 'clear-buffer search' now clears the query instead of parking an unread map entry -- it previously worked only by accident of the effect's special case, and no keymap binds it, so no user-visible change.","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-jjx","title":"Scrolled grid suppresses first visible row-header label","description":"src/ui/grid.rs:338-350: repeat-suppression shows a row-header sub-label only when ri == 0 or the prefix differs from data_row_items[ri-1]. ri is the absolute data-row index (starts at row_offset, grid.rs:269), so when the view is scrolled the first visible row compares against the off-screen row above and its label is blanked — the user loses the group context at the top of the viewport. Fix: compare against the previous *visible* row, i.e. always show labels for the first rendered data row. Found in systematic review 2026-06-09.","status":"closed","priority":3,"issue_type":"bug","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-06-10T03:58:38Z","created_by":"Edward Langley","updated_at":"2026-06-10T04:46:47Z","closed_at":"2026-06-10T04:46:47Z","close_reason":"Fixed (test-first where applicable): records_display returns None for OOB columns (f04fe51); try_axis_of added + axis_of invariant documented + cycle_axis no longer panics on unregistered categories (f04fe51); scrolled header suppression fixed via pure show_sublabel helper for both row and column headers (df9a02b); CyclePanelFocus cycles from current panel with wrap (9e02245); AGENTS.md merge target corrected (9634c95).","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-byz","title":"records_display returns Some(\"\") for out-of-bounds columns instead of None","description":"crates/improvise-core/src/view/layout.rs:186-202: records_display() calls col_label(col), which returns an empty string for out-of-bounds indices; the empty string then fails the == \"Value\" check, is looked up as a category name, and the function returns Some(\"\") via unwrap_or_default(). cell_key() (~line 396) explicitly returns None for an empty col_label — records_display should be symmetric so view/data mismatches are not masked. Found in systematic review 2026-06-09.","status":"closed","priority":3,"issue_type":"bug","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-06-10T03:58:35Z","created_by":"Edward Langley","updated_at":"2026-06-10T04:46:47Z","closed_at":"2026-06-10T04:46:47Z","close_reason":"Fixed (test-first where applicable): records_display returns None for OOB columns (f04fe51); try_axis_of added + axis_of invariant documented + cycle_axis no longer panics on unregistered categories (f04fe51); scrolled header suppression fixed via pure show_sublabel helper for both row and column headers (df9a02b); CyclePanelFocus cycles from current panel with wrap (9e02245); AGENTS.md merge target corrected (9634c95).","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-zgb","title":".improv formula loader mis-splits category suffix via rfind(\" [\")","description":"crates/improvise-io/src/persistence/mod.rs:403-411: the formulas_section walker finds the [Category] suffix with raw.rfind(\" [\") + ends_with(']'). Two failure modes: (1) a pipe-quoted identifier at the end of the formula containing ' [' (e.g. |Revenue [USD]|) is mis-split, treating part of the quoted name as the category; (2) a suffix written without a leading space (X=Y[Cat]) is not recognized and the whole line is parsed as a _Measure formula, losing the target category. Fix: move the category suffix into the pest grammar instead of string scanning. Write failing tests first per AGENTS.md. Found in systematic review 2026-06-09.","status":"closed","priority":3,"issue_type":"bug","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-06-10T03:58:32Z","created_by":"Edward Langley","updated_at":"2026-06-10T04:45:53Z","closed_at":"2026-06-10T04:45:53Z","close_reason":"Fixed in 3418162/47ad9e6 (test-first): export_csv via csv::Writer honoring number format; malformed numbers -\u003e CellValue::Error; formula [Category] suffix moved into pest grammar; parse errors report line/col + offending line. 155 io tests green.","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
@@ -164,6 +171,8 @@
|
||||
{"_type":"issue","id":"improvise-l36","title":"3.2 Publish to crates.io","description":"After cargo publish --dry-run is clean and user confirms, run cargo publish. Verify crate appears at crates.io/crates/improvise and cargo install improvise works.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-04-09T04:07:58Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:28:48Z","dependencies":[{"issue_id":"improvise-l36","depends_on_id":"improvise-11a","type":"blocks","created_at":"2026-04-08T21:09:29Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-l36","depends_on_id":"improvise-2fr","type":"blocks","created_at":"2026-04-08T21:09:28Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":2,"dependent_count":2,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-11a","title":"3.1 Configure cargo dist","description":"Run cargo dist init targeting x86_64-unknown-linux-gnu, aarch64-apple-darwin, x86_64-apple-darwin. Skip Windows and musl. Commit generated .github/workflows/release.yml and Cargo.toml additions. Test with v0.1.0-rc1 tag, verify builds, delete rc tag.","status":"closed","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-04-09T04:07:55Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-11T07:49:57Z","close_reason":"Closed","dependencies":[{"issue_id":"improvise-11a","depends_on_id":"improvise-abz","type":"blocks","created_at":"2026-04-08T21:09:28Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":1,"dependent_count":3,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-0s6","title":"Phase 3: Distribution","description":"Configure cargo dist, publish to crates.io, tag v0.1.0 release. Produces prebuilt binaries for Linux x86_64 and macOS (Intel + Apple Silicon).","status":"open","priority":3,"issue_type":"feature","owner":"el-github@elangley.org","created_at":"2026-04-09T04:05:51Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:28:48Z","dependencies":[{"issue_id":"improvise-0s6","depends_on_id":"improvise-11a","type":"blocks","created_at":"2026-04-08T23:37:39Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-0s6","depends_on_id":"improvise-3tj","type":"blocks","created_at":"2026-04-08T23:37:41Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-0s6","depends_on_id":"improvise-l36","type":"blocks","created_at":"2026-04-08T23:37:40Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":3,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-0vh1","title":"number_text emits grammar-rejected 'inf' and panics on NaN","description":"From the wave-1 gp-tree review (2026-09-19). Both are guarded by documented preconditions, hence P4.\n\nimprovise-gp/src/lib.rs:124. number_text(f32::INFINITY): at p=0 it yields \"inf\", and \"inf\".parse::\u003cf64\u003e() succeeds, so the round-trip loop ACCEPTS it and emits 'inf' -- which the formula grammar rejects (number has no such form). fitted_formula then returns Err, which is fine, but a bare formula_text caller gets silently unparseable text. number_text(f32::NAN) never matches (NaN != NaN) and falls through to unreachable!(), i.e. panics.\n\nAlso a doc imprecision worth correcting while there (lib.rs:22-25 and 114-123): the comment says it emits 'the exact decimal value of the f32'. It emits the shortest decimal that ROUND-TRIPS to the same f64 -- which is exactly what the routing argument needs, and is a weaker and more accurate claim. Verified: over 20,000 random f32 bit patterns plus the extremes, every value found a width inside 0..=149, worst observed width 60, none emitted e/E/inf/NaN, all parsed and round-tripped.","status":"open","priority":4,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T20:43:13Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:43:13Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-k099","title":"AppMode::hint_text/label put terminal UI strings on a serde wire enum","description":"From the wave-1 review (2026-09-19). Pre-existing, introduced by improvise-2hi, not by this wave -- filed so it is tracked rather than rediscovered.\n\ngotcha 26's principle is that wire tokens are not UI labels. But AppMode::hint_text and AppMode::label are English, terminal-specific, keybinding-naming UI strings hanging off an enum that is serialized into the client Snapshot. A non-terminal or localized client cannot use them and has to map AppMode itself anyway.\n\nThe established fix shape in this repo is the AxisColor/MinibufColor one: keep a semantic value in the portable crate and let the render layer resolve it (invariant(semantic_palette_at_the_core_boundary)). Decide whether hints are worth the same treatment or whether they should simply move out of the protocol crate.","status":"open","priority":4,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T20:42:38Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:42:38Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-n3wt","title":"prune_axis_items: keep-slice length invariant is only a debug_assert","description":"From the wave-1 review (2026-09-19). NOT a defect today -- filed so the latent risk is recorded rather than rediscovered.\n\nimprovise-ai7 replaced the old guard 'data_idx \u003c rc \u0026\u0026 keep_row[data_idx]' with 'keep.get(data_idx).copied().unwrap_or(false)' plus a debug_assert_eq!(data_idx, keep.len()). The reviewer confirmed the lengths cannot diverge today: keep_row/keep_col are built as (0..rc)/(0..cc) collections, rc is row_count() which is itself data_items(\u0026row_items).count(), nothing mutates the axis vectors between building keep and consuming it, and the fn is private with exactly two call sites. The debug_assert can never fire, and cargo test --workspace --release (assert compiled out) is green.\n\nThe risk is what happens IF a third caller ever derives keep from something other than row_count()/col_count(), because debug_assert is compiled out in release and the two builds then differ in OPPOSITE directions from the old code:\n * keep too SHORT: old code panicked (loud, index out of bounds); new code silently drops the item in release -- a crash becomes silent data loss from the grid.\n * keep too LONG: old dropped every item at index \u003e= rc; new keeps them if flagged.\n\nFix by making the count unsupplied-able rather than asserted -- e.g. have prune_axis_items derive keep's length from items itself, or take a newtype that can only be built from the same vector. Failing that, a doc-comment on the fn naming the invariant and both failure directions.","status":"open","priority":4,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T20:41:44Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:41:44Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-th7m","title":"Dead F(1) binding: bound to bare 'enter-mode' then immediately rebound","description":"Noticed while working improvise-s13 (2026-09-19), not touched per AGENTS.md (mention dead code, do not delete unasked).\n\nIn crates/improvise-command/src/command/keymap.rs, F(1) is bound to a bare \"enter-mode\" with NO args -- which would fail require_args if it were ever dispatched -- and is then rebound on the very next line with [\"help\"]. The first line is dead: it can never take effect.\n\nTrivial deletion, but worth confirming the second binding is the intended one rather than deleting the wrong line.","status":"open","priority":4,"issue_type":"chore","owner":"el-github@elangley.org","created_at":"2026-09-19T20:36:45Z","created_by":"Edward Langley","updated_at":"2026-09-19T20:36:45Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"_type":"issue","id":"improvise-2zq","title":"Records mode has no inline way to add a category, unlike the + column for measures","description":"improvise-c5v gave records mode a trailing '+' column: type a name, the measure is created, and it becomes a real column you can immediately type values into — without leaving the grid. There is no equivalent for categories, which is an asymmetry the wide-format work introduced.\n\nWHAT EXISTS TODAY (all leave the grid):\n- N in normal or records mode -\u003e search-or-category-add (command/cmd/search.rs:172). Emits SetPanelOpen{Category} + enter-mode category-add, so it opens the side panel and switches mode. Records-normal inherits it from the normal keymap (keymap.rs:645 builds rn with_parent(normal)).\n- n inside the category panel -\u003e the same CategoryAdd mode.\n- :add-cat Region (alias for add-category).\n\nNone of them set a coordinate on the row you were standing on, so after adding 'Region' you still have to navigate to that column and type the value.\n\nWHY IT LOOKS TRACTABLE:\n- GridLayout::records_columns filters on cat.kind.is_regular() and does NOT require the category to have items, so a freshly added empty category appears as a records column immediately.\n- CellKey::with(cat, item) ADDS a coordinate when absent, so the existing rename_record_coordinate path already handles a row that has no coordinate for the new category — it will move every measure cell in the row onto the new coordinate, which is the correct semantics.\n\nSo the shape could mirror '+': a second trailing column (say '+cat') whose commit runs add-category and then, optionally, sets that coordinate for the current row.\n\nOPEN QUESTIONS, decide before building:\n1. Two trailing columns ('+' for measure, '+cat' for category) may be more clutter than it is worth. An alternative is one '+' column that disambiguates by prefix, or a key rather than a column.\n2. Adding a category is more consequential than adding a measure: every existing record lacks a coordinate for it, so the column is blank everywhere and MAX_CATEGORIES (12) applies. Adding a measure only adds a blank column. Worth deciding whether inline creation is even desirable, or whether the panel is the right home for a schema change of that weight.\n3. Should creating the category also set its value on the current row in one commit, or just create the column?\n\nFiled after the user asked whether such a bead existed; it did not.","status":"open","priority":4,"issue_type":"feature","owner":"el-github@elangley.org","created_at":"2026-08-29T19:32:40Z","created_by":"Edward Langley","updated_at":"2026-08-29T19:33:23Z","dependencies":[{"issue_id":"improvise-2zq","depends_on_id":"improvise-c5v","type":"blocks","created_at":"2026-08-29T12:32:45Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0}
|
||||
|
||||
+25
-16
@@ -145,7 +145,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
|
||||
1. **Records mode is wide format.** A row is one *coordinate tuple* — the cells sharing everything but `_Measure` — and each raw measure gets its own editable column, followed by a `+` column that creates a new measure (improvise-c5v). Consequences: two measures on one coordinate are ONE row; editing a category column moves **every** measure cell in the row, not just the one under the cursor; `_Dim` in the synthetic key carries the column **index**, not a label, because a category and a measure may share a name — resolve it with `GridLayout::record_col` into a `RecordCol`. Formula targets are not columns (nothing to type into them). Drill pending edits are keyed by `(row, RecordCol)`.
|
||||
2. **Effects never touch `std::fs`.** Every document read or write goes through `App::storage: Box<dyn Storage>` (improvise-fuj). The host picks the backend: `App::new` supplies `PhysicalFs`, `App::with_storage` takes any other — a browser worker will supply OPFS (improvise-i34), and tests use `MemoryFs`. `improvise-io` keeps path-taking wrappers (`save`, `load`, `export_csv`, `parse_csv_with_warnings`) for the native CLI, each delegating to a `_to`/`_from`/`_bytes` variant that takes the backend or the bytes.
|
||||
3. **Every effect declares an `EffectKind`, and there is no default.** `ViewOnly` never reaches other clients; `Model(CellData | LayoutGeometry | WholeWorkbook)` does (improvise-mae). A new effect that forgets to classify itself fails to compile — that is deliberate, since the failure mode is silent: a mis-declared `ViewOnly` edit would simply never reach connected clients. Watch two traps: the *View* lives inside `model_state.workbook`, so "touches model_state" does NOT mean "is a model change" (axis/page/format are `LayoutGeometry`, cursor and scroll are `ViewOnly` — see improvise-8uz); and `Save`/`SaveAs`/`MarkDirty` touch `model_state` for bookkeeping only and are `ViewOnly`.
|
||||
3. **Every effect declares an `EffectKind`, and there is no default.** `ViewOnly` never reaches other clients; `Model(CellData | LayoutGeometry | WholeWorkbook)` does (improvise-mae). A new effect that forgets to classify itself fails to compile — that is deliberate, since the failure mode is silent: a mis-declared `ViewOnly` edit would simply never reach connected clients. Watch two traps: the *View* lives inside `model_state.workbook`, so "touches model_state" does NOT mean "is a model change" (axis/page/format are `LayoutGeometry`, cursor and scroll are `ViewOnly` — see improvise-8uz); and `Save`/`SaveAs`/`MarkDirty` touch `model_state` for bookkeeping only and are `ViewOnly`. `AddCategory`/`RemoveCategory` are the third trap and go the other way: they look like data edits, but both go through `Workbook`, whose `on_category_added`/`on_category_removed` rewrite **every** view's axis map, so they are `LayoutGeometry`. The two "an effect declaring X actually changes X" tests cannot separate them — a category change moves model data *and* view config, so it clears either bar; `structural_effects_declare_their_kind` pins the declaration itself.
|
||||
4. **Commands never mutate.** `&CmdContext` is read-only; return `Vec<Box<dyn Effect>>`. If you want to touch `&mut App`, add or extend an Effect.
|
||||
5. **`CellKey` is always sorted.** Use the constructors; equality and hashing rely on canonical form.
|
||||
6. **No `Model::add_item`.** With a `Workbook` in hand use the facade: `wb.add_item("Region", "East")?` / `add_item_in_group` / `remove_item` (improvise-tvt). With only a `Model`, go through the category: `m.category_mut("Region").unwrap().add_item("East")`.
|
||||
@@ -153,12 +153,12 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
8. **Display rounding is view-only.** `format_f64` (half-away-from-zero) is only called in rendering. Formula eval uses full `f64`. Never feed a rounded value back into eval.
|
||||
9. **Formula eval is fixed-point.** `recompute_formulas(none_cats)` iterates until stable, bounded by `MAX_EVAL_DEPTH`; circular refs converge to `CellValue::Error("circular")`. `App::new` runs it before the first frame so formula cells render on startup.
|
||||
10. **Drill into formula cells** strips the `_Measure` coordinate when it names a formula target, so `matching_cells` returns the raw records that feed the formula instead of returning empty.
|
||||
11. **Minibuffer keymaps come from `TextEntry`.** The seven text-entry modes are built by the `TextEntry` builder in `keymap.rs`, which binds the shared spine (AnyChar→`append-char <buf>`, Backspace→`pop-char <buf>`, Esc→`[clear-buffer <buf>, enter-mode <exit>]`) and welds every commit key to its `clear-buffer` via `.commit(key, cmd, args)` (improvise-w3q). `TextEntry::new` takes the `AppMode` and reads `<buf>` off its `MinibufferConfig`, so the buffer the prompt names and the buffer the keys edit are the same string by construction (improvise-k8h) — the keymap never spells a buffer name. `.rebind` exists only for CommandMode's Backspace. `SearchMode` is hand-written on purpose — no commit key, and `exit-search-mode` owns the query. `MinibufferConfig::buffer_key` is a `String`, not `&'static str`: a borrowed-'static field makes `Deserialize` unimplementable, and every text-entry `AppMode` variant carries a `MinibufferConfig` (improvise-a2x). A buffer named at runtime is read with `cmd::core::read_buffer` and written with its mirror `write_buffer` — "search" is not in the buffers map but its own `ViewState` field, and those two functions are the only code that knows it (improvise-gdl).
|
||||
11. **Minibuffer keymaps come from `TextEntry`.** The seven text-entry modes are built by the `TextEntry` builder in `keymap.rs`, which binds the shared spine (AnyChar→`append-char <buf>`, Backspace→`pop-char <buf>`, Esc→`[clear-buffer <buf>, enter-mode <exit>]`) and welds every commit key to its `clear-buffer` via `.commit(key, cmd, args)` (improvise-w3q). `TextEntry::new` takes the `AppMode` and reads `<buf>` off its `MinibufferConfig`, so the buffer the prompt names and the buffer the keys edit are the same string by construction (improvise-k8h) — the keymap never spells a buffer name. `.rebind` exists only for CommandMode's Backspace. `SearchMode` is hand-written on purpose — no commit key, and `exit-search-mode` owns the query. `EnterMode` clears the incoming mode's minibuffer through `write_buffer` too — building `SetBuffer` directly there was unreachable only because no shipped `AppMode` is keyed `"search"`. `MinibufferConfig::buffer_key` is a `String`, not `&'static str`: a borrowed-'static field makes `Deserialize` unimplementable, and every text-entry `AppMode` variant carries a `MinibufferConfig` (improvise-a2x). A buffer named at runtime is read with `cmd::core::read_buffer` and written with its mirror `write_buffer` — "search" is not in the buffers map but its own `ViewState` field, and those two functions are the only code that knows it (improvise-gdl).
|
||||
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). `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.
|
||||
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.
|
||||
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`. It hard-codes `EffectKind::ViewOnly`: an effect other clients must see keeps a hand-written `impl` so gotcha 3's "no default kind" rule still bites (improvise-8fy). 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.
|
||||
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). 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 is currently mixed** — `=`/`!=` use a `1e-10` epsilon but the div-by-zero guard checks `rv == 0.0` exactly. Don't assume one or the other; tracked for cleanup.
|
||||
18. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this.
|
||||
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.
|
||||
@@ -169,7 +169,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
|
||||
24. **The cursor is per-client and keyed by view.** `ViewState::cursors: HashMap<String, ViewCursor>` holds `selected`/`row_offset`/`col_offset`, keyed by view name; an absent key means the origin. Reach it through `App::cursor()` (a `Copy` snapshot), `App::cursor_mut()`, and `App::reset_cursor()` — never off `View`, which no longer has the fields (improvise-8uz). Two consequences. **Changing the grid's shape must reset the cursor explicitly**: `View::transpose_axes`/`cycle_axis` used to do it themselves, and now `TransposeAxes`/`CycleAxis` call `app.reset_cursor()` — a new geometry effect that renumbers rows or columns has to do the same. **Replacing the document must clear the map**: `LoadModel` and `AcceptWizardModel` call `cursors.clear()`, because the incoming workbook's view names have nothing to do with the outgoing one's. That clearing replaces `Workbook::normalize_view_state`, which is gone — a `View` can no longer carry a stale offset.
|
||||
|
||||
25. **`ViewState` and its field types are `Serialize`/`Deserialize`.** It is the wire payload of the client Snapshot (improvise-q08), so `AppMode`, `MinibufferConfig`, `MinibufColor`, `ViewFrame`, `KeyCode` and `KeyModifiers` all derive both (improvise-a2x), and `improvise-command` depends on `serde` for it. The constraint this puts on new state: **no borrowed-`'static` fields anywhere ViewState can reach** — `Deserialize` cannot produce one, and the error surfaces on every enclosing variant rather than at the offending field. `view_state_survives_a_json_round_trip` guards it.
|
||||
25. **`ViewState` and its field types are `Serialize`/`Deserialize`.** It is the wire payload of the client Snapshot (improvise-q08), so `AppMode`, `MinibufferConfig`, `MinibufColor`, `ViewFrame`, `KeyCode` and `KeyModifiers` all derive both (improvise-a2x), and `improvise-command` depends on `serde` for it. The constraint this puts on new state: **no borrowed-`'static` fields anywhere ViewState can reach** — `Deserialize` cannot produce one, and the error surfaces on every enclosing variant rather than at the offending field. `view_state_survives_a_json_round_trip` guards it. `ViewState` and `Snapshot` also derive `Clone` and `PartialEq`, so the bootstrap message can be compared in a round-trip test.
|
||||
|
||||
26. **Axis presentation lives on `Axis`, colors resolve in `draw.rs`.** Four vocabularies hang off the enum in `view/axis.rs` — `plain_label()` ("Row"/"Col"/"Page"/"None"), `glyph()` (which `Display` appends, giving "Row ↕"), `short_label()` (three columns for the tile bar; `None` is "·", *not* a truncation), and `initial()` (one letter for the view panel; `None` is empty). Plus `color() -> AxisColor` and `next()` for the Row→Column→Page→None rotation. `AxisColor` is a semantic palette: `improvise-core` must not name a ratatui type, so `draw.rs::axis_fg` does the one mapping to a terminal color, exactly as `minibuf_fg` does for `MinibufColor` (improvise-rml). The tile bar and category panel each carried their own copy of the color table before, with nothing keeping them in agreement. Not covered here: `persistence/format.rs` maps `Axis` to the on-disk keywords ("row"/"column"/"page"), which is a format concern and stays in the io layer.
|
||||
|
||||
@@ -183,7 +183,11 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
|
||||
31. **`:` commands settle their mode by observation, not prediction.** `ExecuteCommand` ends every batch with `ReturnToNormalUnlessMoved(ctx.mode)`, which drops to `Normal` only if the mode is still what the batch started in. There is no `Effect::changes_mode()` any more: it was a per-type constant, and `ViewBack`/`ViewForward`/`StartImportWizard` swap the mode only on some runs, so no up-front answer was right — `:import` opened the wizard and was snapped straight back out (improvise-toc). A new effect that changes the mode needs no annotation; just don't return the batch's starting mode from a success path.
|
||||
|
||||
32. **There is no aggregate render cache — each widget folds its own stream.** `improvise-protocol::render` holds one cache type per widget (`GridCache`, `CategoryTreeCache`, `FormulaCache`, `ViewListCache`, `TileBarCache`), each implementing `WidgetCache` with its own `Update` type, and a `Projection` is one such update addressed to one widget (improvise-rkeq). `reduce_view` folds only `ViewState` now — it no longer takes a cache, and `Command::Project` passes through it untouched. Two consequences. **The overlap between caches is deliberate**: the tile bar and the category panel both carry `(category, axis)` and are updated by separate projections, so neither is woken by the other's changes; do not "fix" it by sharing a variant. **Bootstrap is not a second shape**: `Snapshot { view, seed: Vec<Projection> }` seeds a new client with a batch of the same projections the steady stream carries, so there is no whole-state type that could disagree with the incremental one. Nothing is defined for the help overlay (its content is static and `help_page` is already in `ViewState`) or for the import wizard (its projection shape waits on improvise-764 deciding where the wizard state lives).
|
||||
32. **There is no aggregate render cache — each widget folds its own stream.** `improvise-protocol::render` holds one cache type per widget (`GridCache`, `CategoryTreeCache`, `FormulaCache`, `ViewListCache`, `TileBarCache`), each implementing `WidgetCache` with its own `Update` type, and a `Projection` is one such update addressed to one widget (improvise-rkeq). `reduce_view` folds only `ViewState` now — it no longer takes a cache, and `Command::Project` passes through it untouched. Two consequences. **The overlap between caches is deliberate**: the tile bar and the category panel both carry `(category, axis)` and are updated by separate projections, so neither is woken by the other's changes; do not "fix" it by sharing a variant. **Bootstrap is a *compaction*, not a replay**: `Snapshot { view, seed: Vec<Projection> }` seeds a new client with a batch of the same projections the steady stream carries, so there is no whole-state type that could disagree with the incremental one. The seed is the server's *current* state, not the head of the history — `GridCache::seed()` builds it, and the property that earns it is `GridCache::folded(&cache.seed()) == *cache`. Nothing is defined for the help overlay (its content is static and `help_page` is already in `ViewState`) or for the import wizard (its projection shape waits on improvise-764 deciding where the wizard state lives).
|
||||
|
||||
33. **The grid's labels frame its cells, and `apply` enforces it — the fold is not commutative.** Three consequences in `improvise-protocol/src/render.rs`. (a) A `GridUpdate::Cells` that overtakes the `RowLabels`/`ColLabels` naming its positions is **dropped**, because `fit_to_labels` truncates to the labels it has; a client must not reorder the stream, and `a_cell_update_that_overtakes_its_labels_is_lost` pins it. (b) A label change that *renames* a position blanks the cell under it: `fit_to_labels` preserves by POSITION, which is only right where the label did not change, so `apply` calls `invalidate_relabelled` itself rather than trusting a producer to send `GridUpdate::Invalidated` first. `Invalidated` therefore now means only "contents stale, frame unchanged". (c) `col_widths` is re-fitted with the columns — a new column is drawn at its label's own character count until `ColWidths` catches up — because a renderer zips widths with labels and a length mismatch indexes off the end. There is still **no producer** in the tree; this is the shape `from_app` must meet. Note the fold tests: asserting `fold(prefix)+fold(rest) == fold(whole)` is vacuous (it is the definition of a left fold), so the tests that earn their place are the order-sensitivity one and the compaction-equivalence one — see `doctrine(fold_tests_must_test_order_not_the_axiom)`.
|
||||
|
||||
34. **A formula target is unquoted, and pipe quoting has one home.** `parse_formula` strips the pipes off a target exactly as it does off an `Expr::Ref` — the target names a `_Measure` item, so a target that kept its delimiters made `add_formula` create an item literally named `|Total Revenue|`: a ghost column the user's real item never matched, and `Expr::Ref("Total Revenue")` never resolved to. Both the TUI formula minibuffer (`AddFormula`) and `.improv` load go through that one call, so both were affected. The writer emits `Formula::raw`, not `target`, so file text is unaffected either way — which is exactly why the round-trip tests never saw it, and why the fix repairs existing files on next load with no migration. The writing side of the convention is `improvise_formula::quote::quote_name`; `improvise-gp` re-exports it, and `improvise-io/src/persistence/quoting.rs` still carries a third copy whose keyword guard is absent. The guard is the canonical behaviour: bare keywords do parse today, but only because `agg_call` needs a `(` and PEG backtracking falls through to `ref_expr` — rule ordering, not a stated rule.
|
||||
|
||||
---
|
||||
|
||||
@@ -206,7 +210,11 @@ format.rs 229 / 29t format_f64, parse_number_format (display onl
|
||||
|
||||
### `improvise-formula` (`crates/improvise-formula/src/`)
|
||||
```
|
||||
parser.rs 1080 / 65t pest grammar + tokenizer → Formula AST
|
||||
parser.rs 1109 / 67t pest grammar + tokenizer → Formula AST
|
||||
quote.rs 115 / 5t quote_name / is_bare_name: CL-style |…| quoting, the
|
||||
writing half of the grammar's bare_ident/pipe_quoted.
|
||||
improvise-gp re-exports it; improvise-io still has its
|
||||
own unguarded copy in persistence/quoting.rs
|
||||
ast.rs 77 Expr, BinOp, AggFunc, Formula, Filter
|
||||
```
|
||||
|
||||
@@ -232,12 +240,13 @@ import/csv_parser.rs 300 / 8t CSV parsing, multi-file merge
|
||||
|
||||
### `improvise-protocol` (`crates/improvise-protocol/src/`)
|
||||
```
|
||||
lib.rs 472 / 1t ViewState, ViewCursor, ViewFrame, AppMode (16), Panel,
|
||||
lib.rs 483 / 1t ViewState, ViewCursor, ViewFrame, AppMode (16), Panel,
|
||||
MinibufferConfig, MinibufColor, HELP_PAGE_COUNT
|
||||
reduce.rs 301 / 8t Command (the wire format), HelpPage, reduce_view
|
||||
render.rs 590 / 11t CellDisplay, WidgetCache trait + one cache per widget
|
||||
reduce.rs 325 / 8t Command (the wire format), HelpPage, reduce_view
|
||||
render.rs 838 / 17t CellDisplay, WidgetCache trait + one cache per widget
|
||||
(GridCache/CategoryTreeCache/FormulaCache/ViewListCache/
|
||||
TileBarCache), Projection, bootstrap-only Snapshot
|
||||
TileBarCache), GridCache::seed (the bootstrap
|
||||
compaction), Projection, bootstrap-only Snapshot
|
||||
key.rs 101 / 3t KeyCode, KeyModifiers, KeyEvent (moved from command/)
|
||||
```
|
||||
|
||||
@@ -258,25 +267,25 @@ improvise-gp/src/lib.rs ~330 / 7t formula_text / fitted_formula: a Tree as
|
||||
### Command layer (`crates/improvise-command/src/command/`)
|
||||
```
|
||||
cmd/core.rs 319 / 2t Cmd trait, CmdContext, CmdRegistry, parse helpers
|
||||
cmd/registry.rs 604 / 0t default_registry() — all registrations (no tests yet)
|
||||
cmd/registry.rs 758 / 3t default_registry() — all registrations
|
||||
cmd/navigation.rs 534 / 12t Move, EnterAdvance, Page*
|
||||
cmd/cell.rs 198 / 6t ClearCell, YankCell, PasteCell, TransposeAxes, SaveCmd
|
||||
cmd/commit.rs 517 / 9t CommitAndAdvance, CommitBuffer(CommitKind), CommitExport
|
||||
cmd/effect_cmds.rs 466 / 5t effect_cmd! macro, 25+ simple wrappers
|
||||
cmd/grid.rs 519 / 9t ToggleGroup, ViewNavigate, DrillIntoCell, TogglePruneEmpty
|
||||
cmd/mode.rs 354 / 8t EnterMode, Quit, EditOrDrill, EnterTileSelect
|
||||
cmd/mode.rs 394 / 11t EnterMode, Quit, EditOrDrill, EnterTileSelect
|
||||
cmd/panel.rs 587 / 13t Panel toggle/cycle/cursor, formula/category/view panels
|
||||
cmd/search.rs 202 / 4t SearchNavigate, SearchOrCategoryAdd, ExitSearchMode
|
||||
cmd/text_buffer.rs 240 / 7t AppendChar, PopChar, CommandModeBackspace, ExecuteCommand
|
||||
cmd/tile.rs 160 / 5t MoveTileCursor, TileAxisOp
|
||||
cmd/wizard.rs 199 / 4t HandleWizardKey — per-step import-wizard key tables
|
||||
keymap.rs 1597 / 32t KeyPattern, Binding, Keymap, TextEntry, ModeKey, 16 mode keymaps
|
||||
keymap.rs 1747 / 33t KeyPattern, Binding, Keymap, TextEntry, ModeKey, 16 mode keymaps
|
||||
parse.rs 236 / 19t Script/command-line parser (prefix syntax)
|
||||
```
|
||||
|
||||
### Semantic UI state (`crates/improvise-command/src/ui/`)
|
||||
```
|
||||
ui/effect.rs 2568 / 61t Effect trait, 50+ effect types
|
||||
ui/effect.rs 2644 / 63t Effect trait, 50+ effect types
|
||||
ui/app.rs 2134 / 71t App, ModelState, ViewState, AppMode (16) + dispatch methods, Panel, handle_key, autosave, HELP_PAGE_COUNT
|
||||
ui/cat_tree.rs 156 / 6t Category tree flattener for panel
|
||||
ui/viewport.rs 128 compute_col_widths / row_header_width / visible_cols (pure geometry)
|
||||
|
||||
@@ -782,7 +782,7 @@ fn run_fit(csv_path: Option<String>, save_path: Option<String>, opts: FitOpts, o
|
||||
if let Some(path) = save_path {
|
||||
let model = Model {
|
||||
target_name: target_name.clone(),
|
||||
seed: rng.seed(),
|
||||
seed: Some(rng.seed()),
|
||||
columns,
|
||||
feature_names: names,
|
||||
target: model,
|
||||
|
||||
@@ -99,10 +99,13 @@ pub enum TargetModel {
|
||||
#[derive(Serialize, Deserialize)]
|
||||
pub struct Model {
|
||||
pub target_name: String,
|
||||
/// The RNG seed the fit ran under, so the same fit can be re-run. Defaults
|
||||
/// to 0 when reading a model saved before the seed was recorded.
|
||||
/// The RNG seed the fit ran under, so the same fit can be re-run. `None`
|
||||
/// for a model saved before the seed was recorded: `0` is a legal seed, so
|
||||
/// a sentinel would make an unknown seed indistinguishable from a fit that
|
||||
/// genuinely ran at 0, and anything reporting it would state a seed that
|
||||
/// never ran.
|
||||
#[serde(default)]
|
||||
pub seed: u64,
|
||||
pub seed: Option<u64>,
|
||||
pub columns: Vec<ColumnSpec>,
|
||||
pub feature_names: Vec<String>,
|
||||
pub target: TargetModel,
|
||||
@@ -524,6 +527,83 @@ pub fn encode_row(columns: &[ColumnSpec], header: &[String], row: &[String]) ->
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
|
||||
/// A `Model` populated so every field family carries a non-default value:
|
||||
/// both `ColumnKind` shapes that impute, a one-hot list, and a real tree.
|
||||
fn a_saved_model(seed: Option<u64>) -> Model {
|
||||
Model {
|
||||
target_name: "Score".to_string(),
|
||||
seed,
|
||||
columns: vec![
|
||||
ColumnSpec {
|
||||
name: "amount".to_string(),
|
||||
kind: ColumnKind::Numeric {
|
||||
impute: 2.5,
|
||||
has_missing: true,
|
||||
},
|
||||
},
|
||||
ColumnSpec {
|
||||
name: "outlook".to_string(),
|
||||
kind: ColumnKind::Categorical {
|
||||
values: vec!["rainy".to_string(), MISSING_CAT.to_string()],
|
||||
},
|
||||
},
|
||||
],
|
||||
feature_names: vec![
|
||||
"amount".to_string(),
|
||||
"amount.missing".to_string(),
|
||||
"outlook=rainy".to_string(),
|
||||
],
|
||||
target: TargetModel::Regress {
|
||||
mean: 1.5,
|
||||
std: 0.5,
|
||||
tree: Tree::Split {
|
||||
feat: 0,
|
||||
thresh: 2.0,
|
||||
lo: Box::new(Tree::Leaf(-1.0)),
|
||||
hi: Box::new(Tree::Leaf(1.0)),
|
||||
},
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_saved_model_round_trips_with_its_seed() {
|
||||
let json = serde_json::to_string(&a_saved_model(Some(0xABCD_1234))).unwrap();
|
||||
let back: Model = serde_json::from_str(&json).unwrap();
|
||||
assert_eq!(back.seed, Some(0xABCD_1234));
|
||||
assert_eq!(back.target_name, "Score");
|
||||
assert_eq!(back.feature_names.len(), 3);
|
||||
assert!(
|
||||
matches!(back.columns[1].kind, ColumnKind::Categorical { ref values } if values.len() == 2)
|
||||
);
|
||||
}
|
||||
|
||||
/// A model saved before the seed was recorded must still load — nothing
|
||||
/// pinned that, and `#[serde(default)]` is easy to drop in a cleanup.
|
||||
#[test]
|
||||
fn a_model_saved_before_seeds_still_loads() {
|
||||
let json = serde_json::to_string(&a_saved_model(None)).unwrap();
|
||||
let mut value: serde_json::Value = serde_json::from_str(&json).unwrap();
|
||||
value
|
||||
.as_object_mut()
|
||||
.expect("a model is a JSON object")
|
||||
.remove("seed");
|
||||
let back: Model = serde_json::from_value(value).unwrap();
|
||||
assert_eq!(back.seed, None, "an absent seed is unknown, not seed 0");
|
||||
}
|
||||
|
||||
/// `0` is a legal seed, so "unknown" and "fitted at 0" have to be two
|
||||
/// different values — anything reporting the seed of a pre-seed model
|
||||
/// would otherwise state one that never ran.
|
||||
#[test]
|
||||
fn seed_zero_is_distinguishable_from_no_seed() {
|
||||
let zero: Model =
|
||||
serde_json::from_str(&serde_json::to_string(&a_saved_model(Some(0))).unwrap()).unwrap();
|
||||
assert_eq!(zero.seed, Some(0));
|
||||
assert_ne!(zero.seed, a_saved_model(None).seed);
|
||||
}
|
||||
|
||||
use super::*;
|
||||
|
||||
fn table(header: &[&str], rows: &[&[&str]]) -> (Vec<String>, Vec<Vec<String>>) {
|
||||
|
||||
@@ -80,15 +80,25 @@ impl SplitMix64 {
|
||||
pub fn seed(&self) -> u64 {
|
||||
self.seed
|
||||
}
|
||||
}
|
||||
|
||||
impl Rng for SplitMix64 {
|
||||
fn range(&mut self, lo: i32, hi: i32) -> i32 {
|
||||
/// The raw generator: one step of SplitMix64.
|
||||
///
|
||||
/// Public because it is what `splitmix64_matches_the_reference_vectors`
|
||||
/// pins. The constants below are the algorithm, not an implementation
|
||||
/// detail — every seed already printed by a fit and every `data::Model`
|
||||
/// seed already saved is a promise that *these* numbers come back.
|
||||
pub fn next_u64(&mut self) -> u64 {
|
||||
self.state = self.state.wrapping_add(0x9E37_79B9_7F4A_7C15);
|
||||
let mut z = self.state;
|
||||
z = (z ^ (z >> 30)).wrapping_mul(0xBF58_476D_1CE4_E5B9);
|
||||
z = (z ^ (z >> 27)).wrapping_mul(0x94D0_49BB_1331_11EB);
|
||||
z ^= z >> 31;
|
||||
z ^ (z >> 31)
|
||||
}
|
||||
}
|
||||
|
||||
impl Rng for SplitMix64 {
|
||||
fn range(&mut self, lo: i32, hi: i32) -> i32 {
|
||||
let z = self.next_u64();
|
||||
if hi <= lo {
|
||||
lo
|
||||
} else {
|
||||
@@ -594,6 +604,46 @@ mod tests {
|
||||
assert!(next.iter().all(|t| is_valid(t, 4)));
|
||||
}
|
||||
|
||||
/// The vendored RNG's constants had nothing pinning them: changing the
|
||||
/// gamma from `0x9E3779B97F4A7C15` to `…7C17` left every other test in the
|
||||
/// workspace green, because they only ever assert that two runs at one
|
||||
/// seed agree. But the crate's promise is stronger — a fit's seed is
|
||||
/// printed to the user and saved in `data::Model`, so a *later* run at an
|
||||
/// old seed must reproduce the old fit. That makes these the published
|
||||
/// SplitMix64 vectors for seed 0, not a golden capture of this code.
|
||||
#[test]
|
||||
fn splitmix64_matches_the_reference_vectors() {
|
||||
let mut rng = SplitMix64::new(0);
|
||||
let got: Vec<u64> = (0..5).map(|_| rng.next_u64()).collect();
|
||||
assert_eq!(
|
||||
got,
|
||||
[
|
||||
0xE220_A839_7B1D_CDAF,
|
||||
0x6E78_9E6A_A1B9_65F4,
|
||||
0x06C4_5D18_8009_454F,
|
||||
0xF88B_B8A8_724C_81EC,
|
||||
0x1B39_896A_51A8_749B,
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
/// The same, from a non-zero seed: seed 0 alone would not catch a change
|
||||
/// to how the seed enters the state (`state = seed` vs. `state = 0`).
|
||||
#[test]
|
||||
fn splitmix64_matches_the_reference_vectors_from_a_nonzero_seed() {
|
||||
let mut rng = SplitMix64::new(0x0F17_C0FF_EE00_D00D);
|
||||
let got: Vec<u64> = (0..3).map(|_| rng.next_u64()).collect();
|
||||
assert_eq!(
|
||||
got,
|
||||
[
|
||||
0xBB9C_B85F_8F15_A540,
|
||||
0x04EC_2248_ED37_ECA3,
|
||||
0xD8F6_7264_D9E6_4D26,
|
||||
]
|
||||
);
|
||||
assert_eq!(rng.seed(), 0x0F17_C0FF_EE00_D00D, "the seed is remembered");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn population_serde_round_trips() {
|
||||
let mut rng = SplitMix64::new(1);
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
use crate::ui::app::AppMode;
|
||||
use crate::ui::effect::{self, Effect};
|
||||
|
||||
use super::core::{Cmd, CmdContext};
|
||||
use super::core::{Cmd, CmdContext, write_buffer};
|
||||
use super::grid::DrillIntoCell;
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -58,6 +58,36 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-gdl made `write_buffer` the single accessor for a
|
||||
/// runtime-named buffer, because "search" is not an entry in the buffers
|
||||
/// map but its own `ViewState` field. `EnterMode` built a `SetBuffer`
|
||||
/// directly, so a mode whose `buffer_key` is "search" would clear a map
|
||||
/// entry nobody reads and leave the previous query on screen — the exact
|
||||
/// desync gdl closed. No shipped `AppMode` names that buffer yet, so the
|
||||
/// mode here is constructed by hand; that is what makes the test a
|
||||
/// demonstration rather than a restatement.
|
||||
#[test]
|
||||
fn enter_mode_clears_a_search_keyed_minibuffer_through_write_buffer() {
|
||||
use crate::ui::app::{MinibufColor, MinibufferConfig};
|
||||
|
||||
let m = two_cat_model();
|
||||
let layout = make_layout(&m);
|
||||
let reg = make_registry();
|
||||
let ctx = make_ctx(&m, &layout, ®);
|
||||
let mode = AppMode::Editing {
|
||||
minibuf: MinibufferConfig {
|
||||
buffer_key: "search".into(),
|
||||
prompt: "search: ".into(),
|
||||
color: MinibufColor::Green,
|
||||
},
|
||||
};
|
||||
let dbg = effects_debug(&EnterMode(mode).execute(&ctx));
|
||||
assert!(
|
||||
dbg.contains("SetSearchQuery"),
|
||||
"a \"search\"-keyed minibuffer must clear the search query, got: {dbg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn force_quit_always_produces_quit_mode() {
|
||||
let m = two_cat_model();
|
||||
@@ -202,12 +232,12 @@ impl Cmd for EnterMode {
|
||||
}
|
||||
fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
|
||||
// Clear the corresponding buffer when entering a text-entry mode
|
||||
// Clear the corresponding buffer when entering a text-entry mode.
|
||||
// Through `write_buffer`, which is the only code that knows a
|
||||
// runtime-named buffer may not be an entry in the buffers map
|
||||
// (improvise-gdl).
|
||||
if let Some(mb) = self.0.minibuffer() {
|
||||
effects.push(Box::new(effect::SetBuffer {
|
||||
name: mb.buffer_key.to_string(),
|
||||
value: String::new(),
|
||||
}));
|
||||
effects.push(write_buffer(&mb.buffer_key, String::new()));
|
||||
}
|
||||
effects.push(effect::change_mode(self.0.clone()));
|
||||
effects
|
||||
|
||||
@@ -950,6 +950,7 @@ impl KeymapSet {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::command::cmd::test_helpers;
|
||||
|
||||
// ── Keymap::lookup fallback chain ──────────────────────────────────
|
||||
|
||||
@@ -1211,26 +1212,45 @@ mod tests {
|
||||
assert!(search.lookup(KeyCode::Esc, KeyModifiers::NONE).is_some());
|
||||
}
|
||||
|
||||
/// improvise-9cn: `Keymap::dispatch` resolves a command name through the
|
||||
/// registry with `.ok()?`, so a binding naming a command that is not
|
||||
/// registered is a silent no-op key. This walks every binding in every
|
||||
/// mode and requires the name to resolve — which also catches a registry
|
||||
/// entry disappearing (a mis-variant prototype colliding with its
|
||||
/// sibling) while a keymap still binds the vanished name.
|
||||
/// improvise-9cn: `Keymap::dispatch` resolves a binding through
|
||||
/// `registry.interactive(name, args, ctx).ok()?`, so a binding is a
|
||||
/// silent no-op key in two ways, not one: the name may not be
|
||||
/// registered, *or* the args may not parse. Both walks live here.
|
||||
///
|
||||
/// The args half matters because improvise-s13 moved bindings out of the
|
||||
/// nullary class the name check covers: `enter-mode ["export-prompt"]`
|
||||
/// and `enter-mode ["formula-edit"]` carry a mode name that
|
||||
/// `parse_mode_name` must accept, and a typo like `"export_prompt"`
|
||||
/// compiles, registers, and produces a dead `Ctrl-e`.
|
||||
#[test]
|
||||
fn every_bound_command_name_is_registered() {
|
||||
let ks = KeymapSet::default_keymaps();
|
||||
let registered: Vec<&str> = ks.registry().names().collect();
|
||||
let wb = test_helpers::two_cat_model();
|
||||
let layout = test_helpers::make_layout(&wb);
|
||||
let mut checked = 0usize;
|
||||
|
||||
let require = |name: &str, mode: &ModeKey| {
|
||||
assert!(
|
||||
registered.contains(&name),
|
||||
"{mode:?} binds \"{name}\", which no registry entry provides"
|
||||
);
|
||||
};
|
||||
|
||||
for (mode, km) in &ks.mode_maps {
|
||||
// Build the context in the mode whose keymap this is: a few
|
||||
// parsers read `ctx.mode` (`commit-buffer item` resolves its
|
||||
// target category off `AppMode::ItemAdd`).
|
||||
let app_mode = app_mode_for(mode);
|
||||
let mut ctx = test_helpers::make_ctx(&wb, &layout, ks.registry());
|
||||
ctx.mode = &app_mode;
|
||||
|
||||
let mut require = |name: &str, args: &[String]| {
|
||||
assert!(
|
||||
registered.contains(&name),
|
||||
"{mode:?} binds \"{name}\", which no registry entry provides"
|
||||
);
|
||||
if let Err(e) = ks.registry().interactive(name, args, &ctx) {
|
||||
panic!(
|
||||
"{mode:?} binds \"{name}\" with args {args:?}, which will not parse: {e}"
|
||||
);
|
||||
}
|
||||
checked += 1;
|
||||
};
|
||||
|
||||
for key in probe_keys() {
|
||||
for mods in [
|
||||
KeyModifiers::NONE,
|
||||
@@ -1239,14 +1259,10 @@ mod tests {
|
||||
KeyModifiers::ALT,
|
||||
] {
|
||||
match km.lookup(key, mods) {
|
||||
Some(Binding::Cmd { name, .. }) => {
|
||||
require(name, mode);
|
||||
checked += 1;
|
||||
}
|
||||
Some(Binding::Cmd { name, args }) => require(name, args),
|
||||
Some(Binding::Sequence(steps)) => {
|
||||
for (name, _) in steps {
|
||||
require(name, mode);
|
||||
checked += 1;
|
||||
for (name, args) in steps {
|
||||
require(name, args);
|
||||
}
|
||||
}
|
||||
Some(Binding::Prefix(_)) | None => {}
|
||||
@@ -1257,6 +1273,123 @@ mod tests {
|
||||
assert!(checked > 100, "expected to walk the keymaps, saw {checked}");
|
||||
}
|
||||
|
||||
/// improvise-2hi: `AppMode::hint_text` is the status bar's promise about
|
||||
/// what the keys do, and nothing tied it to the keymaps.
|
||||
/// `only_quit_has_no_hint` asserts a hint is *present*, never that it is
|
||||
/// *accurate*, so rebinding or dropping a key leaves the hint quietly
|
||||
/// lying to the user. This walks every `Key:verb` token in every hint and
|
||||
/// requires the key to resolve in that mode's keymap.
|
||||
///
|
||||
/// The hint is the source of truth for *which* keys are worth mentioning;
|
||||
/// the keymap is the source of truth for what they do. Only the first
|
||||
/// half is checkable from here — a hint that says `i:edit` where `i`
|
||||
/// enters records mode still passes.
|
||||
#[test]
|
||||
fn every_hint_key_resolves_in_its_mode() {
|
||||
let ks = KeymapSet::default_keymaps();
|
||||
let mut checked = 0usize;
|
||||
for (mode, km) in &ks.mode_maps {
|
||||
// SearchMode is Normal plus a flag and has no hint of its own;
|
||||
// `app_mode_for` hands back Normal's, which describes Normal.
|
||||
if *mode == ModeKey::SearchMode {
|
||||
continue;
|
||||
}
|
||||
let hint = app_mode_for(mode).hint_text();
|
||||
for (key, mods) in hint_keys(hint) {
|
||||
assert!(
|
||||
km.lookup(key, mods).is_some(),
|
||||
"{mode:?} hints {hint:?}, but {key:?}+{mods:?} is bound to nothing there"
|
||||
);
|
||||
checked += 1;
|
||||
}
|
||||
}
|
||||
assert!(checked > 50, "expected to walk the hints, saw {checked}");
|
||||
}
|
||||
|
||||
/// Every key a hint mentions. Anything it cannot classify is a panic,
|
||||
/// not a skip: a hint token this does not understand is exactly the
|
||||
/// token that would go unchecked.
|
||||
fn hint_keys(hint: &str) -> Vec<(KeyCode, KeyModifiers)> {
|
||||
// Everything after the em dash is prose ("— type a category name").
|
||||
let head = hint.split('—').next().unwrap_or_default();
|
||||
let mut keys = Vec::new();
|
||||
let mut seen_hint = false;
|
||||
for token in head.split_whitespace() {
|
||||
match token.rsplit_once(':') {
|
||||
// ":q", ":w" — CommandMode lists ex-commands, not keys.
|
||||
// Normal's "::cmd" splits as (":", "cmd") and is a key.
|
||||
Some(("", _)) => seen_hint = true,
|
||||
Some((spec, _)) => {
|
||||
keys.extend(hint_key_spec(spec, hint));
|
||||
seen_hint = true;
|
||||
}
|
||||
// A colon-less word continues the previous hint's verb
|
||||
// ("Enter:add & continue", ":q quit"). One with nothing to
|
||||
// continue is a token this test does not understand.
|
||||
None => assert!(
|
||||
seen_hint,
|
||||
"hint token {token:?} in {hint:?} is not a Key:verb hint"
|
||||
),
|
||||
}
|
||||
}
|
||||
keys
|
||||
}
|
||||
|
||||
/// The key-spec dialect the hints use: `Enter`, `Ctrl-e`, `F/C/V` for
|
||||
/// alternatives, and a bare run of characters (`hjkl`, `[:]`) otherwise.
|
||||
fn hint_key_spec(spec: &str, hint: &str) -> Vec<(KeyCode, KeyModifiers)> {
|
||||
let plain = |code| vec![(code, KeyModifiers::NONE)];
|
||||
match spec {
|
||||
"" => panic!("empty key spec in hint {hint:?}"),
|
||||
_ if spec.contains('/') => spec
|
||||
.split('/')
|
||||
.flat_map(|s| hint_key_spec(s, hint))
|
||||
.collect(),
|
||||
"Enter" => plain(KeyCode::Enter),
|
||||
"Esc" => plain(KeyCode::Esc),
|
||||
"Tab" => plain(KeyCode::Tab),
|
||||
"S-Tab" => plain(KeyCode::BackTab),
|
||||
"Space" => plain(KeyCode::Char(' ')),
|
||||
"BS" => plain(KeyCode::Backspace),
|
||||
"Del" => plain(KeyCode::Delete),
|
||||
_ => match spec
|
||||
.strip_prefix("Ctrl-")
|
||||
.map(|r| r.chars().collect::<Vec<_>>())
|
||||
{
|
||||
Some(rest) if rest.len() == 1 => {
|
||||
vec![(KeyCode::Char(rest[0]), KeyModifiers::CONTROL)]
|
||||
}
|
||||
Some(_) => panic!("unreadable key spec {spec:?} in hint {hint:?}"),
|
||||
None => spec
|
||||
.chars()
|
||||
.map(|c| (KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.collect(),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/// A representative `AppMode` for each keymap, so a test can put a
|
||||
/// `CmdContext` in the mode whose bindings it is walking.
|
||||
fn app_mode_for(mode: &ModeKey) -> AppMode {
|
||||
match mode {
|
||||
ModeKey::Normal | ModeKey::SearchMode => AppMode::Normal,
|
||||
ModeKey::Help => AppMode::Help,
|
||||
ModeKey::FormulaPanel => AppMode::FormulaPanel,
|
||||
ModeKey::CategoryPanel => AppMode::CategoryPanel,
|
||||
ModeKey::ViewPanel => AppMode::ViewPanel,
|
||||
ModeKey::TileSelect => AppMode::TileSelect,
|
||||
ModeKey::Editing => AppMode::editing(),
|
||||
ModeKey::FormulaEdit => AppMode::formula_edit(),
|
||||
ModeKey::CategoryAdd => AppMode::category_add(),
|
||||
ModeKey::ItemAdd => AppMode::item_add("Type".to_string()),
|
||||
ModeKey::ExportPrompt => AppMode::export_prompt(),
|
||||
ModeKey::CommandMode => AppMode::command_mode(),
|
||||
ModeKey::ImportWizard => AppMode::ImportWizard,
|
||||
ModeKey::RecordsNormal => AppMode::RecordsNormal,
|
||||
ModeKey::RecordsEditing => AppMode::records_editing(),
|
||||
}
|
||||
}
|
||||
|
||||
/// The key space the binding walks cover: every printable ASCII plus the
|
||||
/// named keys the keymaps actually use.
|
||||
fn probe_keys() -> Vec<KeyCode> {
|
||||
|
||||
@@ -56,19 +56,25 @@ pub trait Effect: Debug {
|
||||
/// Declare a setter effect: a one-field newtype whose `apply` writes that
|
||||
/// field into one place on the `App`.
|
||||
///
|
||||
/// Every effect declared here is `ViewOnly`, and that is the point of the
|
||||
/// macro rather than an accident of its first callers: a setter whose write
|
||||
/// other clients must see is not simple, and gets a hand-written `impl` that
|
||||
/// says so out loud (gotcha 3). Within that boundary the macro is the single
|
||||
/// hook for anything these setters should all do — today none of them marks
|
||||
/// the document dirty or rebuilds the layout, and the macro makes that one
|
||||
/// decision instead of fifteen silent ones (improvise-8fy).
|
||||
/// The kind is a **mandatory** argument, even though every call site so far
|
||||
/// answers `ViewOnly`. Hard-coding it reintroduced exactly the default that
|
||||
/// `Effect::kind` deliberately does without (gotcha 3): `$place` is an
|
||||
/// unconstrained expression, so
|
||||
/// `simple_effect!(X(String) => |app| app.model_state.workbook.active_view_mut().number_format, …)`
|
||||
/// is a textbook `LayoutGeometry` write that the macro would have reported as
|
||||
/// `ViewOnly`, silently, with the whole suite green. Writing the answer per
|
||||
/// call site is the same reasoning that gave `kind()` no default.
|
||||
///
|
||||
/// Within that, the macro is still the single hook for anything these setters
|
||||
/// should all do — today none of them marks the document dirty or rebuilds the
|
||||
/// layout, and the macro makes that one decision instead of fifteen silent
|
||||
/// ones (improvise-8fy).
|
||||
///
|
||||
/// `Clone::clone` rather than `.clone()`: the field is `usize` or `bool` as
|
||||
/// often as it is a `String`, and the call form keeps `clippy::clone_on_copy`
|
||||
/// off the generated code.
|
||||
macro_rules! simple_effect {
|
||||
($(#[$meta:meta])* $name:ident($ty:ty) => |$app:ident| $place:expr) => {
|
||||
($(#[$meta:meta])* $name:ident($ty:ty) => |$app:ident| $place:expr, $kind:expr) => {
|
||||
$(#[$meta])*
|
||||
#[derive(Debug)]
|
||||
pub struct $name(pub $ty);
|
||||
@@ -77,7 +83,7 @@ macro_rules! simple_effect {
|
||||
$place = Clone::clone(&self.0);
|
||||
}
|
||||
fn kind(&self) -> EffectKind {
|
||||
EffectKind::ViewOnly
|
||||
$kind
|
||||
}
|
||||
}
|
||||
};
|
||||
@@ -94,7 +100,7 @@ impl Effect for AddCategory {
|
||||
}
|
||||
}
|
||||
fn kind(&self) -> EffectKind {
|
||||
EffectKind::Model(ModelChange::CellData)
|
||||
EffectKind::Model(ModelChange::LayoutGeometry)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -311,7 +317,7 @@ impl Effect for RemoveCategory {
|
||||
app.model_state.workbook.remove_category(&self.0);
|
||||
}
|
||||
fn kind(&self) -> EffectKind {
|
||||
EffectKind::Model(ModelChange::CellData)
|
||||
EffectKind::Model(ModelChange::LayoutGeometry)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -539,13 +545,13 @@ impl Effect for SetSelected {
|
||||
}
|
||||
}
|
||||
|
||||
simple_effect!(SetRowOffset(usize) => |app| app.cursor_mut().row_offset);
|
||||
simple_effect!(SetRowOffset(usize) => |app| app.cursor_mut().row_offset, EffectKind::ViewOnly);
|
||||
|
||||
simple_effect!(SetColOffset(usize) => |app| app.cursor_mut().col_offset);
|
||||
simple_effect!(SetColOffset(usize) => |app| app.cursor_mut().col_offset, EffectKind::ViewOnly);
|
||||
|
||||
// ── App state ────────────────────────────────────────────────────────────────
|
||||
|
||||
simple_effect!(ChangeMode(AppMode) => |app| app.view_state.mode);
|
||||
simple_effect!(ChangeMode(AppMode) => |app| app.view_state.mode, EffectKind::ViewOnly);
|
||||
|
||||
/// Return to `Normal` unless something earlier in this batch already moved
|
||||
/// the mode away from `from` — the mode the batch started in.
|
||||
@@ -570,7 +576,7 @@ impl Effect for ReturnToNormalUnlessMoved {
|
||||
}
|
||||
}
|
||||
|
||||
simple_effect!(SetStatus(String) => |app| app.view_state.status_msg);
|
||||
simple_effect!(SetStatus(String) => |app| app.view_state.status_msg, EffectKind::ViewOnly);
|
||||
|
||||
#[derive(Debug)]
|
||||
pub struct MarkDirty;
|
||||
@@ -583,11 +589,11 @@ impl Effect for MarkDirty {
|
||||
}
|
||||
}
|
||||
|
||||
simple_effect!(SetYanked(Option<CellValue>) => |app| app.view_state.yanked);
|
||||
simple_effect!(SetYanked(Option<CellValue>) => |app| app.view_state.yanked, EffectKind::ViewOnly);
|
||||
|
||||
simple_effect!(SetSearchQuery(String) => |app| app.view_state.search_query);
|
||||
simple_effect!(SetSearchQuery(String) => |app| app.view_state.search_query, EffectKind::ViewOnly);
|
||||
|
||||
simple_effect!(SetSearchMode(bool) => |app| app.view_state.search_mode);
|
||||
simple_effect!(SetSearchMode(bool) => |app| app.view_state.search_mode, EffectKind::ViewOnly);
|
||||
|
||||
/// Set a named buffer's contents.
|
||||
///
|
||||
@@ -611,7 +617,7 @@ impl Effect for SetBuffer {
|
||||
}
|
||||
}
|
||||
|
||||
simple_effect!(SetTileCatIdx(usize) => |app| app.view_state.tile_cat_idx);
|
||||
simple_effect!(SetTileCatIdx(usize) => |app| app.view_state.tile_cat_idx, EffectKind::ViewOnly);
|
||||
|
||||
/// Populate the drill state with a frozen snapshot of records.
|
||||
/// Clears any previous drill state.
|
||||
@@ -1359,6 +1365,7 @@ mod tests {
|
||||
Box::new(ChangeMode(AppMode::Help)),
|
||||
Box::new(SetStatus("hi".into())),
|
||||
Box::new(MarkDirty),
|
||||
Box::new(SetYanked(Some(CellValue::Number(1.0)))),
|
||||
Box::new(SetSearchQuery("q".into())),
|
||||
Box::new(SetSearchMode(true)),
|
||||
Box::new(SetBuffer {
|
||||
@@ -1408,13 +1415,85 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// The structural effects, each pinned to the kind it must declare.
|
||||
///
|
||||
/// The bug this guards: `AddCategory` and `RemoveCategory` declared
|
||||
/// `CellData`, but both go through `Workbook`, whose `on_category_added`
|
||||
/// / `on_category_removed` rewrite **every** view's axis map. That is a
|
||||
/// re-layout, so a client told only "cell data changed" would keep
|
||||
/// rendering the old grid shape. The two "actually change" tests below
|
||||
/// cannot see it — a category change moves model data *and* view config,
|
||||
/// so it passes either list. Only pinning the declaration does.
|
||||
#[test]
|
||||
fn structural_effects_declare_their_kind() {
|
||||
let cell = ModelChange::CellData;
|
||||
let geom = ModelChange::LayoutGeometry;
|
||||
let cases: Vec<(Box<dyn Effect>, ModelChange)> = vec![
|
||||
(Box::new(AddCategory("Region".into())), geom),
|
||||
(Box::new(RemoveCategory("Month".into())), geom),
|
||||
(
|
||||
Box::new(AddItem {
|
||||
category: "Type".into(),
|
||||
item: "Drink".into(),
|
||||
}),
|
||||
cell,
|
||||
),
|
||||
(
|
||||
Box::new(AddItemInGroup {
|
||||
category: "Type".into(),
|
||||
item: "Drink".into(),
|
||||
group: "G".into(),
|
||||
}),
|
||||
cell,
|
||||
),
|
||||
(
|
||||
Box::new(RemoveItem {
|
||||
category: "Type".into(),
|
||||
item: "Food".into(),
|
||||
}),
|
||||
cell,
|
||||
),
|
||||
(
|
||||
Box::new(SetCell(
|
||||
CellKey::new(vec![("Type".into(), "Food".into())]),
|
||||
CellValue::Number(1.0),
|
||||
)),
|
||||
cell,
|
||||
),
|
||||
(
|
||||
Box::new(ClearCell(CellKey::new(vec![(
|
||||
"Type".into(),
|
||||
"Food".into(),
|
||||
)]))),
|
||||
cell,
|
||||
),
|
||||
(
|
||||
Box::new(SetAxis {
|
||||
category: "Type".into(),
|
||||
axis: Axis::Page,
|
||||
}),
|
||||
geom,
|
||||
),
|
||||
(Box::new(TransposeAxes), geom),
|
||||
(Box::new(CreateView("Other".into())), geom),
|
||||
(Box::new(SetNumberFormat(",.3f".into())), geom),
|
||||
(Box::new(TogglePruneEmpty), geom),
|
||||
];
|
||||
for (effect, expected) in cases {
|
||||
assert_eq!(
|
||||
effect.kind(),
|
||||
EffectKind::Model(expected),
|
||||
"{effect:?} declares the wrong EffectKind"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// The other direction: effects declaring `CellData` must actually
|
||||
/// change model data, or projection would emit nothing for a real edit.
|
||||
#[test]
|
||||
fn cell_data_effects_actually_change_model_data() {
|
||||
let key = CellKey::new(vec![("Type".into(), "Food".into())]);
|
||||
let cases: Vec<Box<dyn Effect>> = vec![
|
||||
Box::new(AddCategory("Region".into())),
|
||||
Box::new(AddItem {
|
||||
category: "Type".into(),
|
||||
item: "Drink".into(),
|
||||
@@ -1443,6 +1522,8 @@ mod tests {
|
||||
#[test]
|
||||
fn layout_geometry_effects_actually_change_view_config() {
|
||||
let cases: Vec<Box<dyn Effect>> = vec![
|
||||
Box::new(AddCategory("Region".into())),
|
||||
Box::new(RemoveCategory("Month".into())),
|
||||
Box::new(SetAxis {
|
||||
category: "Type".into(),
|
||||
axis: Axis::Page,
|
||||
|
||||
@@ -15,8 +15,9 @@ WHITESPACE = _{ " " | "\t" }
|
||||
|
||||
formula = { SOI ~ target ~ "=" ~ expr ~ where_clause? ~ EOI }
|
||||
|
||||
// The target keeps its raw text (including pipes, if any) — we capture
|
||||
// the span directly rather than walking into its children.
|
||||
// The target is an ordinary identifier: parser.rs unquotes it exactly the
|
||||
// way it unquotes a `ref_expr`, so `|Total Revenue| = ...` and a reference
|
||||
// to `|Total Revenue|` name the same item.
|
||||
target = { identifier }
|
||||
|
||||
where_clause = { ^"WHERE" ~ identifier ~ "=" ~ filter_value }
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
pub mod ast;
|
||||
pub mod parser;
|
||||
pub mod quote;
|
||||
|
||||
pub use ast::{AggFunc, BinOp, Expr, Filter, Formula};
|
||||
pub use parser::parse_formula;
|
||||
pub use quote::{is_bare_name, quote_name};
|
||||
|
||||
@@ -52,7 +52,10 @@ fn build_formula(pair: Pair<Rule>, raw: &str, target_category: &str) -> Formula
|
||||
for inner in pair.into_inner() {
|
||||
let rule = inner.as_rule();
|
||||
if rule == Rule::target {
|
||||
target = Some(inner.as_str().trim().to_string());
|
||||
// Unquoted exactly like `Expr::Ref`: a target and a reference that
|
||||
// spell the same name must produce the same item name, or the
|
||||
// formula's own output lands in a column nothing can refer to.
|
||||
target = Some(identifier_to_string(first_inner(inner)));
|
||||
} else if rule == Rule::expr {
|
||||
expr = Some(build_expr(inner));
|
||||
} else if rule == Rule::where_clause {
|
||||
@@ -538,7 +541,7 @@ mod tests {
|
||||
#[test]
|
||||
fn pipe_quoted_identifier_in_expression() {
|
||||
let f = parse_formula("|Total Revenue| = |Base Revenue| + Bonus", "Foo").unwrap();
|
||||
assert_eq!(f.target, "|Total Revenue|");
|
||||
assert_eq!(f.target, "Total Revenue");
|
||||
assert!(matches!(
|
||||
&f.expr,
|
||||
Expr::BinOp(BinOp::Add, lhs, rhs)
|
||||
@@ -851,10 +854,36 @@ mod tests {
|
||||
));
|
||||
}
|
||||
|
||||
/// Regression: the tree walker took the target's raw span, so a
|
||||
/// pipe-quoted target came back with its delimiters still attached while
|
||||
/// an `Expr::Ref` for the very same name came back unquoted. Downstream
|
||||
/// that is a data bug, not a cosmetic one: `Model::add_formula` creates a
|
||||
/// `_Measure` item literally named `|My Target|`, which never matches the
|
||||
/// user's item `My Target`, so the formula writes into a ghost column and
|
||||
/// nothing referring to it by name resolves.
|
||||
#[test]
|
||||
fn pipe_quoted_target_preserves_pipes() {
|
||||
let f = parse_formula("|My Target| = A", "Cat").unwrap();
|
||||
assert_eq!(f.target, "|My Target|");
|
||||
fn pipe_quoted_target_is_unquoted_like_a_reference() {
|
||||
let f = parse_formula("|My Target| = |My Target|", "Cat").unwrap();
|
||||
assert_eq!(f.target, "My Target");
|
||||
assert!(
|
||||
matches!(&f.expr, Expr::Ref(s) if s == &f.target),
|
||||
"a target and a reference spelled identically must name the same item, got {:?}",
|
||||
f.expr
|
||||
);
|
||||
}
|
||||
|
||||
/// The escapes apply to the target too — the same ones `Expr::Ref` gets.
|
||||
#[test]
|
||||
fn pipe_quoted_target_applies_escapes() {
|
||||
let f = parse_formula(r"|a\|b| = A", "Cat").unwrap();
|
||||
assert_eq!(f.target, "a|b");
|
||||
}
|
||||
|
||||
/// A bare target is unaffected: there is nothing to strip.
|
||||
#[test]
|
||||
fn bare_target_is_unchanged() {
|
||||
let f = parse_formula("Profit = A", "Cat").unwrap();
|
||||
assert_eq!(f.target, "Profit");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,115 @@
|
||||
//! CL-style `|…|` quoting of names — the writing half of the convention
|
||||
//! `formula.pest`'s `bare_ident` / `pipe_quoted` defines for reading.
|
||||
//!
|
||||
//! This lives here because `improvise-formula` owns the grammar that says what
|
||||
//! a bare name is, and because it is what every emitter needs: a name that came
|
||||
//! from a user's data has to be spelled so the parser hands it back unchanged.
|
||||
//! `improvise-gp` carried its own copy; so does
|
||||
//! `improvise-io/src/persistence/quoting.rs`, and the two had already drifted
|
||||
//! over the keyword rule below.
|
||||
|
||||
/// Words the formula grammar gives a meaning of their own. A name spelled like
|
||||
/// one is pipe-quoted even though it is otherwise a legal bare identifier.
|
||||
const KEYWORDS: [&str; 7] = ["WHERE", "SUM", "AVG", "MIN", "MAX", "COUNT", "IF"];
|
||||
|
||||
/// Whether `name` may be written without pipes: it matches
|
||||
/// `[A-Za-z_][A-Za-z0-9_-]*` and is not one of the [`KEYWORDS`].
|
||||
///
|
||||
/// The keyword clause is deliberately stricter than the grammar strictly
|
||||
/// requires. `SUM` does parse bare today, but only because `agg_call` needs a
|
||||
/// `(` and PEG backtracking then falls through to `ref_expr` — an emergent
|
||||
/// property of rule ordering, not a stated rule. A pipe-quoted name is an
|
||||
/// identifier in every position with nothing to fall through, so a name that
|
||||
/// collides with a keyword is quoted and stops depending on that.
|
||||
pub fn is_bare_name(name: &str) -> bool {
|
||||
let mut chars = name.chars();
|
||||
let starts = chars
|
||||
.next()
|
||||
.is_some_and(|c| c.is_ascii_alphabetic() || c == '_');
|
||||
starts
|
||||
&& chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
|
||||
&& !KEYWORDS.iter().any(|k| k.eq_ignore_ascii_case(name))
|
||||
}
|
||||
|
||||
/// `name` as the grammar wants it: bare when [`is_bare_name`] allows, otherwise
|
||||
/// pipe-quoted with the `\|`, `\\` and `\n` escapes `pipe_quoted` defines.
|
||||
pub fn quote_name(name: &str) -> String {
|
||||
if is_bare_name(name) {
|
||||
return name.to_string();
|
||||
}
|
||||
let mut out = String::with_capacity(name.len() + 2);
|
||||
out.push('|');
|
||||
for c in name.chars() {
|
||||
match c {
|
||||
'|' => out.push_str("\\|"),
|
||||
'\\' => out.push_str("\\\\"),
|
||||
'\n' => out.push_str("\\n"),
|
||||
other => out.push(other),
|
||||
}
|
||||
}
|
||||
out.push('|');
|
||||
out
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::{Expr, parse_formula};
|
||||
|
||||
#[test]
|
||||
fn a_bare_name_is_left_alone() {
|
||||
for name in ["Revenue", "_hidden", "east-coast", "a1"] {
|
||||
assert_eq!(quote_name(name), name);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn everything_else_is_pipe_quoted() {
|
||||
assert_eq!(quote_name("Net Revenue"), "|Net Revenue|");
|
||||
assert_eq!(quote_name("9lives"), "|9lives|");
|
||||
assert_eq!(quote_name(""), "||");
|
||||
assert_eq!(quote_name("Cost + Tax"), "|Cost + Tax|");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_keyword_is_quoted_even_though_it_looks_bare() {
|
||||
for name in ["WHERE", "sum", "If", "COUNT"] {
|
||||
assert_eq!(quote_name(name), format!("|{name}|"));
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_escapes_match_the_grammar() {
|
||||
assert_eq!(quote_name("a|b"), r"|a\|b|");
|
||||
assert_eq!(quote_name(r"back\slash"), r"|back\\slash|");
|
||||
assert_eq!(quote_name("two\nlines"), r"|two\nlines|");
|
||||
}
|
||||
|
||||
/// The property the whole module exists for: a quoted name parses, and the
|
||||
/// parser hands back the name that went in — as a target and as a
|
||||
/// reference, which after the target-unquoting fix are the same rule.
|
||||
#[test]
|
||||
fn a_quoted_name_round_trips_through_the_parser() {
|
||||
for name in [
|
||||
"Net Revenue",
|
||||
"WHERE",
|
||||
"SUM",
|
||||
"outlook=rainy",
|
||||
"a|b",
|
||||
r"back\slash",
|
||||
"two\nlines",
|
||||
"9lives",
|
||||
"",
|
||||
] {
|
||||
let text = format!("{} = {}", quote_name(name), quote_name(name));
|
||||
let f = parse_formula(&text, "_Measure")
|
||||
.unwrap_or_else(|e| panic!("{name:?} emitted {text:?}: {e}"));
|
||||
assert_eq!(f.target, name, "target for {name:?}");
|
||||
assert!(
|
||||
matches!(&f.expr, Expr::Ref(got) if got == name),
|
||||
"reference for {name:?}, got {:?}",
|
||||
f.expr
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
+141
-61
@@ -19,17 +19,19 @@
|
||||
//! spaces, operators, pipes and keyword collisions; every name goes through
|
||||
//! [`quote_name`], the same CL-style pipe quoting the formula tokenizer
|
||||
//! accepts.
|
||||
//! * **It routes identically.** A threshold is emitted as the *exact* decimal
|
||||
//! value of the `f32`, not a rounded one, so a row sitting exactly on a
|
||||
//! split boundary falls the same side in `f64` formula evaluation as it does
|
||||
//! in [`gp_tree::eval`]. See [`number_text`].
|
||||
//! * **It routes identically.** A threshold is emitted as the shortest decimal
|
||||
//! that reads back as the same `f64` the `f32` widens to — not a rounded one
|
||||
//! — so a row sitting exactly on a split boundary falls the same side in
|
||||
//! `f64` formula evaluation as it does in [`gp_tree::eval`]. See
|
||||
//! [`number_text`].
|
||||
|
||||
use gp_tree::{Spec, Tree};
|
||||
use improvise_formula::{Formula, parse_formula};
|
||||
|
||||
/// Reserved words the formula tokenizer treats as token boundaries. A feature
|
||||
/// named after one is pipe-quoted so it stays an identifier.
|
||||
const KEYWORDS: [&str; 7] = ["WHERE", "SUM", "AVG", "MIN", "MAX", "COUNT", "IF"];
|
||||
// The quoting rule itself lives in `improvise_formula::quote`, next to the
|
||||
// grammar that defines it — this crate had its own copy, and a third lives in
|
||||
// improvise-io. Re-exported because it was part of this crate's public surface.
|
||||
pub use improvise_formula::quote_name;
|
||||
|
||||
/// The formula text for a fitted tree: `<target> = <nested IFs>`.
|
||||
///
|
||||
@@ -80,55 +82,39 @@ fn expr_text(tree: &Tree, spec: &Spec) -> String {
|
||||
}
|
||||
}
|
||||
|
||||
/// A name as the formula grammar wants it: bare when it matches
|
||||
/// `[A-Za-z_][A-Za-z0-9_-]*` and is not a keyword, otherwise CL-style
|
||||
/// pipe-quoted with `\|`, `\\` and `\n` escapes.
|
||||
pub fn quote_name(name: &str) -> String {
|
||||
if is_bare(name) {
|
||||
return name.to_string();
|
||||
}
|
||||
let mut out = String::with_capacity(name.len() + 2);
|
||||
out.push('|');
|
||||
for c in name.chars() {
|
||||
match c {
|
||||
'|' => out.push_str("\\|"),
|
||||
'\\' => out.push_str("\\\\"),
|
||||
'\n' => out.push_str("\\n"),
|
||||
other => out.push(other),
|
||||
}
|
||||
}
|
||||
out.push('|');
|
||||
out
|
||||
}
|
||||
|
||||
fn is_bare(name: &str) -> bool {
|
||||
let mut chars = name.chars();
|
||||
let starts = chars
|
||||
.next()
|
||||
.is_some_and(|c| c.is_ascii_alphabetic() || c == '_');
|
||||
starts
|
||||
&& chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
|
||||
&& !KEYWORDS.iter().any(|k| k.eq_ignore_ascii_case(name))
|
||||
}
|
||||
|
||||
/// An `f32` as decimal text that reads back as the *same* number.
|
||||
///
|
||||
/// The formula grammar's `number` has no exponent form, so this is plain
|
||||
/// fixed-point notation: the shortest fraction width whose text parses back to
|
||||
/// the identical `f64`. Precision is not cosmetic here. Widening an `f32` to
|
||||
/// `f64` is exact, so emitting the exact value makes `feature < thresh` give
|
||||
/// the same answer in `f64` formula evaluation as in `f32` [`gp_tree::eval`].
|
||||
/// A shortest-round-trip-to-`f32` rendering (`0.1` for `0.1f32`) would sit up
|
||||
/// to half an ULP away, and a row whose feature value equals the threshold
|
||||
/// exactly would then fall on the *other* side of the split.
|
||||
/// the identical `f64`. That is the claim — the shortest decimal that
|
||||
/// *round-trips*, not the f32's full decimal expansion, which runs to 149
|
||||
/// fractional digits and is not what gets emitted.
|
||||
///
|
||||
/// Reading back as the same `f64` is the property the routing argument needs,
|
||||
/// and it is not cosmetic. Widening an `f32` to `f64` is exact, so text that
|
||||
/// reads back as that `f64` makes `feature < thresh` give the same answer in
|
||||
/// `f64` formula evaluation as in `f32` [`gp_tree::eval`]. A
|
||||
/// shortest-round-trip-to-`f32` rendering (`0.1` for `0.1f32`) would sit up to
|
||||
/// half an ULP away, and a row whose feature value equals the threshold exactly
|
||||
/// would then fall on the *other* side of the split.
|
||||
///
|
||||
/// Panics on a non-finite value. The grammar spells neither one, and both fail
|
||||
/// quietly rather than loudly: `"inf"` parses as an `f64`, so the loop below
|
||||
/// would return text no formula parser accepts, and NaN matches no width at
|
||||
/// all. Trees the crate's own operators build over a finite [`Spec`] are
|
||||
/// finite, so this is a precondition, not a runtime condition.
|
||||
fn number_text(v: f32) -> String {
|
||||
assert!(
|
||||
v.is_finite(),
|
||||
"number_text wants a finite value, got {v}: the formula grammar spells neither infinity nor NaN"
|
||||
);
|
||||
let exact = v as f64;
|
||||
// An f32 is a binary fraction, so its decimal expansion terminates — at
|
||||
// 149 fractional digits in the worst (subnormal) case.
|
||||
// A finite f32 is a binary fraction, so its decimal expansion terminates —
|
||||
// at 149 fractional digits in the worst (subnormal) case.
|
||||
(0..=149)
|
||||
.map(|p| format!("{exact:.p$}"))
|
||||
.find(|s| s.parse::<f64>() == Ok(exact))
|
||||
.unwrap_or_else(|| unreachable!("an f32's decimal expansion terminates"))
|
||||
.unwrap_or_else(|| unreachable!("a finite f32's decimal expansion terminates"))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -263,15 +249,72 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// Regression: this test used to assert `parsed.target == "|Total
|
||||
/// Revenue|"` and call the pipes correct. They are not — the target names
|
||||
/// a `_Measure` item, so a target carrying its delimiters writes the fit
|
||||
/// into an item literally named `|Total Revenue|`, which is not the item
|
||||
/// the user has. What the emitter owes its caller is a *semantic* round
|
||||
/// trip: the name goes in, the same name comes back out.
|
||||
#[test]
|
||||
fn an_awkward_target_name_is_quoted_and_round_trips() {
|
||||
let spec = spec_of(&[(0.0, 1.0)], &["x"]);
|
||||
let text = formula_text(&Tree::Leaf(1.0), &spec, "Total Revenue");
|
||||
assert_eq!(text, "|Total Revenue| = 1");
|
||||
let parsed = parse_formula(&text, MEASURE).expect("emitted formula parses");
|
||||
assert_eq!(
|
||||
parsed.target, "|Total Revenue|",
|
||||
"the target keeps its span"
|
||||
assert_eq!(parsed.target, "Total Revenue");
|
||||
}
|
||||
|
||||
/// And the consequence that makes it a data bug rather than a cosmetic
|
||||
/// one: `add_formula` puts the target into `_Measure`, so an unquoted
|
||||
/// target is what makes the fit land in the column the user can see.
|
||||
#[test]
|
||||
fn an_awkward_target_names_the_users_measure_item() {
|
||||
let spec = spec_of(&[(0.0, 1.0)], &["x"]);
|
||||
let formula = fitted_formula(&Tree::Leaf(1.0), &spec, "Total Revenue", MEASURE)
|
||||
.expect("emitted formula parses");
|
||||
let mut model = Model::new("fit");
|
||||
model.add_formula(formula);
|
||||
assert!(
|
||||
model
|
||||
.measure_item_names()
|
||||
.iter()
|
||||
.any(|n| n == "Total Revenue"),
|
||||
"the fit should land in |Total Revenue|, got {:?}",
|
||||
model.measure_item_names()
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression: `number_text(f32::INFINITY)` emitted `"inf"`, which
|
||||
/// `"inf".parse::<f64>()` accepts — so the round-trip loop returned it and
|
||||
/// a `formula_text` caller got text the grammar rejects, with nothing on
|
||||
/// the way saying so. `fitted_formula` would surface it as a parse error;
|
||||
/// a bare `formula_text` caller would write it into a file.
|
||||
#[test]
|
||||
#[should_panic(expected = "finite")]
|
||||
fn an_infinite_threshold_is_refused_rather_than_emitted_as_inf() {
|
||||
number_text(f32::INFINITY);
|
||||
}
|
||||
|
||||
/// Regression: NaN fell through the loop to `unreachable!`, so the panic
|
||||
/// blamed the decimal expansion instead of the caller's value.
|
||||
#[test]
|
||||
#[should_panic(expected = "finite")]
|
||||
fn a_nan_threshold_is_refused_by_name() {
|
||||
number_text(f32::NAN);
|
||||
}
|
||||
|
||||
/// What the emitted text actually is, since the doc used to overclaim it:
|
||||
/// the *shortest* decimal that reads back as the same `f64`, not the f32's
|
||||
/// full decimal expansion. The weaker claim is the one the routing
|
||||
/// argument needs — equal text means equal `f64` means the same branch.
|
||||
#[test]
|
||||
fn a_threshold_is_the_shortest_decimal_that_reads_back_the_same() {
|
||||
let v = 0.7f32;
|
||||
let text = number_text(v);
|
||||
assert_eq!(text.parse::<f64>(), Ok(v as f64), "{text} lost the value");
|
||||
assert!(
|
||||
text.len() < format!("{:.149}", v as f64).len(),
|
||||
"{text} is the full expansion, not the shortest round trip"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -336,27 +379,36 @@ mod tests {
|
||||
assert_eq!(formula.target_category, MEASURE);
|
||||
}
|
||||
|
||||
/// Mean squared error of a tree over the training rows.
|
||||
fn cost(tree: &Tree, rows: &[(Vec<f32>, f32)]) -> f32 {
|
||||
rows.iter()
|
||||
.map(|(f, y)| (eval(tree, f) - y).powi(2))
|
||||
.sum::<f32>()
|
||||
/ rows.len() as f32
|
||||
}
|
||||
|
||||
/// The smallest GA that still makes every kind of random choice: initial
|
||||
/// population, mutation, crossover, immigrants.
|
||||
fn fit(spec: &Spec, rows: &[(Vec<f32>, f32)], seed: u64) -> Tree {
|
||||
/// population, mutation, crossover, immigrants. Returns the best member of
|
||||
/// generation 0 alongside the final fit, so a test can ask whether the
|
||||
/// search actually moved rather than only whether it was repeatable.
|
||||
fn fit_from_gen0(spec: &Spec, rows: &[(Vec<f32>, f32)], seed: u64) -> (Tree, Tree) {
|
||||
let cfg = GaConfig {
|
||||
pop: 16,
|
||||
elite: 4,
|
||||
immigrants: 2,
|
||||
max_nodes: 15,
|
||||
};
|
||||
let cost = |t: &Tree| -> f32 {
|
||||
rows.iter()
|
||||
.map(|(f, y)| (eval(t, f) - y).powi(2))
|
||||
.sum::<f32>()
|
||||
/ rows.len() as f32
|
||||
};
|
||||
let mut rng = SplitMix64::new(seed);
|
||||
let mut pop = Population::seed(&random_tree(spec, 3, &mut rng), cfg.pop, spec, &mut rng);
|
||||
let mut gen0 = None;
|
||||
for _ in 0..20 {
|
||||
let mut scored: Vec<(f32, Tree)> =
|
||||
pop.members.iter().map(|t| (cost(t), t.clone())).collect();
|
||||
let mut scored: Vec<(f32, Tree)> = pop
|
||||
.members
|
||||
.iter()
|
||||
.map(|t| (cost(t, rows), t.clone()))
|
||||
.collect();
|
||||
scored.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap_or(std::cmp::Ordering::Equal));
|
||||
gen0 = gen0.or_else(|| Some(scored[0].1.clone()));
|
||||
pop.members = breed(
|
||||
scored.into_iter().map(|(_, t)| t).collect(),
|
||||
&cfg,
|
||||
@@ -364,7 +416,14 @@ mod tests {
|
||||
&mut rng,
|
||||
);
|
||||
}
|
||||
simplify(&pop.members[0], spec)
|
||||
(
|
||||
gen0.expect("the loop runs at least once"),
|
||||
simplify(&pop.members[0], spec),
|
||||
)
|
||||
}
|
||||
|
||||
fn fit(spec: &Spec, rows: &[(Vec<f32>, f32)], seed: u64) -> Tree {
|
||||
fit_from_gen0(spec, rows, seed).1
|
||||
}
|
||||
|
||||
const FIT_RANGES: [(f32, f32); 2] = [(0.0, 1.0), (0.0, 10.0)];
|
||||
@@ -394,6 +453,11 @@ mod tests {
|
||||
|
||||
/// And the seed is what varies it — otherwise "reproducible" would just mean
|
||||
/// the search never moved.
|
||||
///
|
||||
/// On its own this pair is weak evidence: a GA whose `breed` returned its
|
||||
/// input unchanged would still differ across seeds, because the initial
|
||||
/// `random_tree` already does. `breeding_improves_on_generation_zero` is
|
||||
/// what rules that out.
|
||||
#[test]
|
||||
fn a_different_seed_fits_a_different_formula() {
|
||||
let spec = spec_of(&FIT_RANGES, &FIT_NAMES);
|
||||
@@ -402,4 +466,20 @@ mod tests {
|
||||
let other = formula_text(&fit(&spec, &rows, 0x9999_0001), &spec, TARGET);
|
||||
assert_ne!(first, other);
|
||||
}
|
||||
|
||||
/// The search moved: the fit the user gets is strictly better on the
|
||||
/// training rows than the best tree the initial population offered.
|
||||
#[test]
|
||||
fn breeding_improves_on_generation_zero() {
|
||||
let spec = spec_of(&FIT_RANGES, &FIT_NAMES);
|
||||
let rows = fit_rows();
|
||||
for seed in [0xABCD_1234u64, 0x9999_0001, 7] {
|
||||
let (gen0, fitted) = fit_from_gen0(&spec, &rows, seed);
|
||||
let (before, after) = (cost(&gen0, &rows), cost(&fitted, &rows));
|
||||
assert!(
|
||||
after < before,
|
||||
"seed {seed:#x}: breeding left the cost at {after} (generation 0 was {before})"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -360,7 +360,7 @@ impl AppMode {
|
||||
///
|
||||
/// State failing either test lives on `App` instead, under the
|
||||
/// **session-local** residue tag.
|
||||
#[derive(Debug, Serialize, Deserialize)]
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
pub struct ViewState {
|
||||
pub mode: AppMode,
|
||||
pub status_msg: String,
|
||||
|
||||
@@ -136,8 +136,9 @@ pub fn reduce_view(view: &mut ViewState, cmd: &Command) {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::render::GridUpdate;
|
||||
use crate::render::{CategoryTreeUpdate, CellDisplay, GridUpdate, TileEntry, ViewSummary};
|
||||
use crate::{Panel, ViewCursor};
|
||||
use improvise_core::view::Axis;
|
||||
|
||||
fn fold(cmds: &[Command]) -> ViewState {
|
||||
let mut view = ViewState::default();
|
||||
@@ -290,7 +291,30 @@ mod tests {
|
||||
Command::HelpPage(HelpPage::Set(2)),
|
||||
Command::SetTransientPrefix(Some((KeyCode::Char('g'), KeyModifiers::CONTROL))),
|
||||
Command::SwitchView("Second".into()),
|
||||
// Every arm of the projection envelope, each carrying a payload.
|
||||
// The one payload-free variant, `GridUpdate::Invalidated`, is
|
||||
// the one that proves least about the envelope, so it does not
|
||||
// stand in for the rest (doctrine populate_before_roundtrip).
|
||||
Command::Project(Projection::Grid(GridUpdate::Cells(vec![
|
||||
((0, 1), CellDisplay::number("1,234.50")),
|
||||
((2, 3), CellDisplay::error("#div/0")),
|
||||
]))),
|
||||
Command::Project(Projection::Grid(GridUpdate::ColLabels(vec!["c0".into()]))),
|
||||
Command::Project(Projection::Grid(GridUpdate::ColWidths(vec![8, 12]))),
|
||||
Command::Project(Projection::Grid(GridUpdate::Invalidated)),
|
||||
Command::Project(Projection::CategoryTree(CategoryTreeUpdate::Items {
|
||||
category: "Region".into(),
|
||||
items: vec!["North".into(), "South".into()],
|
||||
})),
|
||||
Command::Project(Projection::Formulas(vec!["Profit = Revenue - Cost".into()])),
|
||||
Command::Project(Projection::ViewList(vec![ViewSummary {
|
||||
name: "Default".into(),
|
||||
axes: vec![("Region".into(), Axis::Row)],
|
||||
}])),
|
||||
Command::Project(Projection::TileBar(vec![TileEntry {
|
||||
category: "Region".into(),
|
||||
axis: Axis::Page,
|
||||
}])),
|
||||
];
|
||||
for cmd in &stream {
|
||||
let json = serde_json::to_string(cmd).expect("serializes");
|
||||
|
||||
@@ -105,15 +105,94 @@ impl GridCache {
|
||||
self.cells.get(row)?.get(col)
|
||||
}
|
||||
|
||||
/// The compaction a server sends to a client that connects late.
|
||||
///
|
||||
/// A server does not replay history — it holds the current state and
|
||||
/// expresses it as the same updates the steady-state stream carries, so
|
||||
/// bootstrap is not a second code path. The property that makes that
|
||||
/// true is `GridCache::folded(&cache.seed()) == *cache`, and it is the
|
||||
/// ordering that earns it: the labels frame the cells, so they go first
|
||||
/// (see [`WidgetCache::apply`]).
|
||||
pub fn seed(&self) -> Vec<GridUpdate> {
|
||||
let cells = self
|
||||
.cells
|
||||
.iter()
|
||||
.enumerate()
|
||||
.flat_map(|(r, row)| {
|
||||
row.iter()
|
||||
.enumerate()
|
||||
.filter(|(_, cell)| *cell != &CellDisplay::default())
|
||||
.map(move |(c, cell)| ((r, c), cell.clone()))
|
||||
})
|
||||
.collect();
|
||||
vec![
|
||||
GridUpdate::RowLabels(self.row_labels.clone()),
|
||||
GridUpdate::ColLabels(self.col_labels.clone()),
|
||||
GridUpdate::ColWidths(self.col_widths.clone()),
|
||||
GridUpdate::Cells(cells),
|
||||
]
|
||||
}
|
||||
|
||||
/// Resize to match the current labels, preserving what still fits.
|
||||
/// The labels frame the cells, so either label update re-fits the grid.
|
||||
/// The labels frame the cells *and the widths*, so every update that
|
||||
/// touches any of the three re-fits the grid — both label arms and
|
||||
/// `ColWidths`. Fitting on the label arms alone was the asymmetry that
|
||||
/// let a widths update put the mismatch back.
|
||||
///
|
||||
/// The widths are framed by the columns too: a column may arrive one
|
||||
/// update ahead of its width, so a new column is drawn at its label's
|
||||
/// own width until `ColWidths` catches up. Letting `col_widths` fall out
|
||||
/// of step with `col_labels` is not a cache that is merely stale — it is
|
||||
/// one a renderer zipping the two indexes off the end of.
|
||||
fn fit_to_labels(&mut self) {
|
||||
let cols = self.col_labels.len();
|
||||
self.cells.resize_with(self.row_labels.len(), Vec::new);
|
||||
for row in &mut self.cells {
|
||||
row.resize_with(cols, CellDisplay::default);
|
||||
}
|
||||
self.col_widths.truncate(cols);
|
||||
for col in self.col_widths.len()..cols {
|
||||
let width = self.col_labels[col].chars().count();
|
||||
self.col_widths.push(width.min(u16::MAX as usize) as u16);
|
||||
}
|
||||
}
|
||||
|
||||
/// Blank every cell framed by a label that is not the one that was
|
||||
/// there before, since the value under it answered the old label.
|
||||
fn invalidate_relabelled(&mut self, old: &[String], axis: LabelAxis) {
|
||||
let new = match axis {
|
||||
LabelAxis::Row => &self.row_labels,
|
||||
LabelAxis::Col => &self.col_labels,
|
||||
};
|
||||
let stale: Vec<usize> = (0..new.len())
|
||||
.filter(|&i| old.get(i) != Some(&new[i]))
|
||||
.collect();
|
||||
for i in stale {
|
||||
match axis {
|
||||
LabelAxis::Row => {
|
||||
if let Some(row) = self.cells.get_mut(i) {
|
||||
row.fill(CellDisplay::default());
|
||||
}
|
||||
}
|
||||
LabelAxis::Col => {
|
||||
for row in &mut self.cells {
|
||||
if let Some(cell) = row.get_mut(i) {
|
||||
*cell = CellDisplay::default();
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Which set of labels a re-fit is reacting to. Private: it exists so the
|
||||
/// row and column cases share one invalidation rule rather than two copies
|
||||
/// that can disagree.
|
||||
#[derive(Debug, Clone, Copy)]
|
||||
enum LabelAxis {
|
||||
Row,
|
||||
Col,
|
||||
}
|
||||
|
||||
/// What can change about the grid: cell values, the labels that frame them,
|
||||
@@ -126,14 +205,22 @@ pub enum GridUpdate {
|
||||
RowLabels(Vec<String>),
|
||||
ColLabels(Vec<String>),
|
||||
ColWidths(Vec<u16>),
|
||||
/// The grid's shape changed and the contents are stale; a `RowLabels` /
|
||||
/// `ColLabels` pair follows.
|
||||
/// The contents are stale although the frame is not — every cell is
|
||||
/// blanked and the labels are kept. A *reshaping* label change does not
|
||||
/// need this: `apply` invalidates the cells it reframes itself.
|
||||
Invalidated,
|
||||
}
|
||||
|
||||
impl WidgetCache for GridCache {
|
||||
type Update = GridUpdate;
|
||||
|
||||
/// Not commutative, and deliberately so: the labels frame the cells, so
|
||||
/// a `Cells` update that overtakes the labels naming its positions is
|
||||
/// dropped rather than guessed at. The corollary is that a label change
|
||||
/// cannot be trusted to leave the cells alone — `fit_to_labels` keeps
|
||||
/// what still fits *by position*, which is only right for the positions
|
||||
/// whose label did not change, so the rest are blanked here rather than
|
||||
/// left for a producer to remember to invalidate.
|
||||
fn apply(&mut self, update: &GridUpdate) {
|
||||
match update {
|
||||
GridUpdate::Cells(changes) => {
|
||||
@@ -144,14 +231,19 @@ impl WidgetCache for GridCache {
|
||||
}
|
||||
}
|
||||
GridUpdate::RowLabels(labels) => {
|
||||
self.row_labels = labels.clone();
|
||||
let old = std::mem::replace(&mut self.row_labels, labels.clone());
|
||||
self.fit_to_labels();
|
||||
self.invalidate_relabelled(&old, LabelAxis::Row);
|
||||
}
|
||||
GridUpdate::ColLabels(labels) => {
|
||||
self.col_labels = labels.clone();
|
||||
let old = std::mem::replace(&mut self.col_labels, labels.clone());
|
||||
self.fit_to_labels();
|
||||
self.invalidate_relabelled(&old, LabelAxis::Col);
|
||||
}
|
||||
GridUpdate::ColWidths(widths) => {
|
||||
self.col_widths = widths.clone();
|
||||
self.fit_to_labels();
|
||||
}
|
||||
GridUpdate::ColWidths(widths) => self.col_widths = widths.clone(),
|
||||
GridUpdate::Invalidated => {
|
||||
// Drop the contents but keep the frame: labels arrive next,
|
||||
// and a stale grid under fresh labels is worse than blank.
|
||||
@@ -321,11 +413,12 @@ pub enum Projection {
|
||||
/// `seed` is a batch of the same projections the steady-state stream
|
||||
/// carries, so bootstrapping is folding: a client applies the seed and then
|
||||
/// keeps folding, and there is no separate whole-state shape that could
|
||||
/// disagree with the incremental one.
|
||||
/// disagree with the incremental one. It is a *compaction* of the history,
|
||||
/// not its head — see [`GridCache::seed`].
|
||||
///
|
||||
/// `view` is the one point at which [`ViewState`] is serialized
|
||||
/// (improvise-q08).
|
||||
#[derive(Debug, Serialize, Deserialize)]
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
pub struct Snapshot {
|
||||
pub view: ViewState,
|
||||
pub seed: Vec<Projection>,
|
||||
@@ -480,54 +573,206 @@ mod tests {
|
||||
assert_eq!(tiles.tiles[0].axis, Axis::Page);
|
||||
}
|
||||
|
||||
/// The fold is a fold: applying a stream one update at a time is the
|
||||
/// same as the end state, whatever the split. This is what lets a client
|
||||
/// start from a `Snapshot` seed and keep folding.
|
||||
/// A column dropped from the middle used to slide the cells to its
|
||||
/// right one place left, because `fit_to_labels` truncates by position:
|
||||
/// `[A,B,C]` shrinking to `[A,C]` left the column now labelled `C`
|
||||
/// showing B's value. Nothing on the wire forced a producer to send an
|
||||
/// `Invalidated` first, so `apply` blanks what it reframes.
|
||||
#[test]
|
||||
fn splitting_a_stream_anywhere_gives_the_same_cache() {
|
||||
let stream = [
|
||||
GridUpdate::RowLabels(vec!["r0".into(), "r1".into()]),
|
||||
GridUpdate::ColLabels(vec!["c0".into(), "c1".into()]),
|
||||
GridUpdate::Cells(vec![((0, 0), CellDisplay::number("1"))]),
|
||||
GridUpdate::ColWidths(vec![8, 12]),
|
||||
GridUpdate::Cells(vec![((1, 1), CellDisplay::error("#div/0"))]),
|
||||
];
|
||||
let whole = GridCache::folded(&stream);
|
||||
for split in 0..=stream.len() {
|
||||
let mut cache = GridCache::folded(&stream[..split]);
|
||||
for update in &stream[split..] {
|
||||
cache.apply(update);
|
||||
}
|
||||
assert_eq!(cache, whole, "splitting after {split} changed the result");
|
||||
}
|
||||
fn a_relabelled_column_does_not_keep_the_old_columns_value() {
|
||||
let mut grid = GridCache::folded(&[
|
||||
GridUpdate::RowLabels(vec!["r0".into()]),
|
||||
GridUpdate::ColLabels(vec!["A".into(), "B".into(), "C".into()]),
|
||||
GridUpdate::Cells(vec![
|
||||
((0, 0), CellDisplay::number("1")),
|
||||
((0, 1), CellDisplay::number("2")),
|
||||
((0, 2), CellDisplay::number("3")),
|
||||
]),
|
||||
]);
|
||||
grid.apply(&GridUpdate::ColLabels(vec!["A".into(), "C".into()]));
|
||||
|
||||
assert_eq!(
|
||||
grid.cell(0, 0).unwrap().text,
|
||||
"1",
|
||||
"column A never moved, so its value stands"
|
||||
);
|
||||
assert_eq!(
|
||||
grid.cell(0, 1).unwrap().text,
|
||||
"",
|
||||
"position 1 is labelled C now; B's value there would be a tear"
|
||||
);
|
||||
}
|
||||
|
||||
/// Bootstrap is not a second code path: seeding from a snapshot and then
|
||||
/// following the stream lands where folding the whole stream lands.
|
||||
/// The same rule on the other axis, and the case a bare label shrink
|
||||
/// gets right by accident: only the *relabelled* positions are blanked,
|
||||
/// so an unchanged row keeps what the server already sent for it.
|
||||
#[test]
|
||||
fn a_snapshot_seed_is_just_the_head_of_the_stream() {
|
||||
let stream = [
|
||||
fn a_label_change_blanks_only_the_positions_it_reframes() {
|
||||
let mut grid = GridCache::folded(&[
|
||||
GridUpdate::RowLabels(vec!["r0".into(), "r1".into()]),
|
||||
GridUpdate::ColLabels(vec!["c0".into()]),
|
||||
GridUpdate::Cells(vec![
|
||||
((0, 0), CellDisplay::number("1")),
|
||||
((1, 0), CellDisplay::number("2")),
|
||||
]),
|
||||
]);
|
||||
grid.apply(&GridUpdate::RowLabels(vec!["r0".into(), "rX".into()]));
|
||||
assert_eq!(grid.cell(0, 0).unwrap().text, "1");
|
||||
assert_eq!(grid.cell(1, 0).unwrap().text, "");
|
||||
}
|
||||
|
||||
/// `col_widths` is framed by the columns: a renderer zips the two, so a
|
||||
/// cache where they disagree in length is one that indexes off the end.
|
||||
/// `ColWidths` is its own update and may be a frame behind the labels.
|
||||
#[test]
|
||||
fn col_widths_always_span_the_column_labels() {
|
||||
let mut grid = GridCache::folded(&[
|
||||
GridUpdate::ColLabels(vec!["A".into()]),
|
||||
GridUpdate::ColWidths(vec![8]),
|
||||
]);
|
||||
grid.apply(&GridUpdate::ColLabels(vec!["A".into(), "Bravo".into()]));
|
||||
assert_eq!(grid.col_widths.len(), grid.col_labels.len());
|
||||
assert_eq!(grid.col_widths[0], 8, "the width already sent is kept");
|
||||
assert_eq!(
|
||||
grid.col_widths[1], 5,
|
||||
"a column with no width yet is drawn at its label's width"
|
||||
);
|
||||
|
||||
grid.apply(&GridUpdate::ColLabels(vec!["A".into()]));
|
||||
assert_eq!(grid.col_widths, vec![8]);
|
||||
}
|
||||
|
||||
/// The same invariant, seen from the other arm. `ColWidths` used to be
|
||||
/// assigned verbatim, so a widths update that disagreed with the labels
|
||||
/// put the cache straight back into the state
|
||||
/// `col_widths_always_span_the_column_labels` exists to forbid — the
|
||||
/// label arm was fitted and the widths arm was not.
|
||||
///
|
||||
/// A short update is the ordinary case: the server resized a column the
|
||||
/// client has not been told about yet. A long one means the widths
|
||||
/// overtook the `ColLabels` that frame them, and the fold drops what
|
||||
/// overtakes its frame — the same rule `Cells` is already held to.
|
||||
#[test]
|
||||
fn a_col_widths_update_is_fitted_to_the_labels_it_is_framed_by() {
|
||||
let mut grid =
|
||||
GridCache::folded(&[GridUpdate::ColLabels(vec!["A".into(), "Bravo".into()])]);
|
||||
|
||||
grid.apply(&GridUpdate::ColWidths(vec![8]));
|
||||
assert_eq!(
|
||||
grid.col_widths.len(),
|
||||
grid.col_labels.len(),
|
||||
"a short widths update must not leave a renderer zipping off the end"
|
||||
);
|
||||
assert_eq!(grid.col_widths[0], 8, "the width that was sent is taken");
|
||||
assert_eq!(
|
||||
grid.col_widths[1], 5,
|
||||
"the column with no width yet keeps its label's width"
|
||||
);
|
||||
|
||||
grid.apply(&GridUpdate::ColWidths(vec![3, 4, 99]));
|
||||
assert_eq!(
|
||||
grid.col_widths,
|
||||
vec![3, 4],
|
||||
"a width for a column that has no label yet is dropped, not stored"
|
||||
);
|
||||
}
|
||||
|
||||
/// This fold is **not** commutative, and a client must not be written as
|
||||
/// though it were. The labels frame the cells, so a `Cells` update that
|
||||
/// overtakes the `ColLabels`/`RowLabels` naming its positions is dropped
|
||||
/// — the same totality that
|
||||
/// `cell_updates_outside_the_cache_are_dropped` pins, seen as an
|
||||
/// ordering contract. Reordering the identical three updates gives a
|
||||
/// different cache.
|
||||
#[test]
|
||||
fn a_cell_update_that_overtakes_its_labels_is_lost() {
|
||||
let cells = GridUpdate::Cells(vec![((0, 0), CellDisplay::number("42"))]);
|
||||
let rows = GridUpdate::RowLabels(vec!["r0".into()]);
|
||||
let cols = GridUpdate::ColLabels(vec!["c0".into()]);
|
||||
|
||||
let in_order = GridCache::folded(&[rows.clone(), cols.clone(), cells.clone()]);
|
||||
let raced = GridCache::folded(&[cells, rows, cols]);
|
||||
|
||||
assert_eq!(in_order.cell(0, 0).unwrap().text, "42");
|
||||
assert_eq!(
|
||||
raced.cell(0, 0).unwrap().text,
|
||||
"",
|
||||
"a cell that arrived before its frame has nowhere to land"
|
||||
);
|
||||
assert_ne!(
|
||||
in_order, raced,
|
||||
"if these agreed, order would not matter and a client could \
|
||||
reorder the stream freely"
|
||||
);
|
||||
}
|
||||
|
||||
/// Bootstrap is not a second code path — but a server does not replay
|
||||
/// history to a late client, it sends the state it holds *now*. So the
|
||||
/// property that matters is compaction: folding the seed must land
|
||||
/// exactly where folding the whole history landed, including the
|
||||
/// history's relabels and overwrites.
|
||||
#[test]
|
||||
fn a_compacted_seed_folds_to_the_same_cache_as_the_whole_history() {
|
||||
let history = [
|
||||
GridUpdate::RowLabels(vec!["r0".into(), "r1".into()]),
|
||||
GridUpdate::ColLabels(vec!["c0".into(), "c1".into()]),
|
||||
GridUpdate::ColWidths(vec![8, 12]),
|
||||
GridUpdate::Cells(vec![((0, 0), CellDisplay::number("1"))]),
|
||||
GridUpdate::Cells(vec![
|
||||
((0, 0), CellDisplay::number("7")),
|
||||
((1, 1), CellDisplay::error("#div/0")),
|
||||
]),
|
||||
GridUpdate::RowLabels(vec!["r0".into(), "r1".into(), "r2".into()]),
|
||||
GridUpdate::Cells(vec![((2, 0), CellDisplay::text("late"))]),
|
||||
];
|
||||
let live = GridCache::folded(&history);
|
||||
|
||||
// The state is worth compacting: overwritten, relabelled, populated.
|
||||
assert_eq!(live.cell(0, 0).unwrap().text, "7");
|
||||
assert!(live.cell(1, 1).unwrap().error);
|
||||
assert_eq!(live.cell(2, 0).unwrap().text, "late");
|
||||
|
||||
let seed = live.seed();
|
||||
assert!(
|
||||
seed.len() < history.len(),
|
||||
"a seed that is not smaller than the history is not a compaction"
|
||||
);
|
||||
assert_eq!(
|
||||
GridCache::folded(&seed),
|
||||
live,
|
||||
"a client seeded from the compaction draws something no client \
|
||||
that watched the whole stream would"
|
||||
);
|
||||
}
|
||||
|
||||
/// And the same property across the envelope: a `Snapshot` seed is a
|
||||
/// compaction of every widget's history, and folding it reproduces
|
||||
/// every widget's cache.
|
||||
#[test]
|
||||
fn a_snapshot_seeds_every_widget_from_its_compacted_state() {
|
||||
let history = [
|
||||
Projection::Grid(GridUpdate::RowLabels(vec!["r0".into()])),
|
||||
Projection::Grid(GridUpdate::ColLabels(vec!["c0".into()])),
|
||||
Projection::Formulas(vec!["Profit = Revenue - Cost".into()]),
|
||||
Projection::Grid(GridUpdate::Cells(vec![((0, 0), CellDisplay::number("5"))])),
|
||||
Projection::Formulas(vec![]),
|
||||
Projection::Formulas(vec!["Tax = Revenue * 0.1".into()]),
|
||||
];
|
||||
let (grid, formulas) = fold_projections(Default::default(), &history);
|
||||
|
||||
let snapshot = Snapshot {
|
||||
view: ViewState::default(),
|
||||
seed: stream[..2].to_vec(),
|
||||
seed: grid
|
||||
.seed()
|
||||
.into_iter()
|
||||
.map(Projection::Grid)
|
||||
.chain([Projection::Formulas(formulas.formulas.clone())])
|
||||
.collect(),
|
||||
};
|
||||
let booted = fold_projections(Default::default(), &snapshot.seed);
|
||||
|
||||
let (booted_grid, booted_formulas) = fold_projections(
|
||||
fold_projections(Default::default(), &snapshot.seed),
|
||||
&stream[2..],
|
||||
);
|
||||
let (whole_grid, whole_formulas) = fold_projections(Default::default(), &stream);
|
||||
|
||||
assert_eq!(booted_grid, whole_grid);
|
||||
assert_eq!(booted_formulas, whole_formulas);
|
||||
assert_eq!(booted_grid.cell(0, 0).unwrap().text, "5");
|
||||
assert!(booted_formulas.formulas.is_empty());
|
||||
assert_eq!(booted, (grid, formulas));
|
||||
assert_eq!(booted.0.cell(0, 0).unwrap().text, "5");
|
||||
assert_eq!(booted.1.formulas, vec!["Tax = Revenue * 0.1"]);
|
||||
}
|
||||
|
||||
/// Two subscribers, each folding only what is addressed to it — the
|
||||
@@ -550,10 +795,15 @@ mod tests {
|
||||
#[test]
|
||||
fn every_projection_round_trips_through_json() {
|
||||
let projections = [
|
||||
Projection::Grid(GridUpdate::Cells(vec![(
|
||||
(0, 1),
|
||||
CellDisplay::error("#div/0"),
|
||||
)])),
|
||||
// One populated value per `CellDisplay` shape: `numeric` decides
|
||||
// the alignment and `error` the styling, so a round trip that
|
||||
// only ever carries them at their default proves nothing about
|
||||
// either.
|
||||
Projection::Grid(GridUpdate::Cells(vec![
|
||||
((0, 1), CellDisplay::number("1,234.50")),
|
||||
((2, 3), CellDisplay::error("#div/0")),
|
||||
((4, 5), CellDisplay::text("pending")),
|
||||
])),
|
||||
Projection::Grid(GridUpdate::RowLabels(vec!["r".into()])),
|
||||
Projection::Grid(GridUpdate::ColLabels(vec!["c".into()])),
|
||||
Projection::Grid(GridUpdate::ColWidths(vec![8, 12])),
|
||||
@@ -587,4 +837,43 @@ mod tests {
|
||||
assert_eq!(&back, projection, "round trip changed {projection:?}");
|
||||
}
|
||||
}
|
||||
|
||||
/// The `Snapshot` is the only thing on the wire that is not an event,
|
||||
/// and the only point at which `ViewState` is serialized (improvise-q08)
|
||||
/// — so it is the one message whose round trip nothing else covers.
|
||||
/// Populated on both halves: a default `ViewState` beside an empty seed
|
||||
/// would round-trip through almost no code.
|
||||
#[test]
|
||||
fn a_populated_snapshot_round_trips_through_json() {
|
||||
let mut view = ViewState {
|
||||
mode: crate::AppMode::item_add("Region".into()),
|
||||
active_view: "Second".into(),
|
||||
transient_prefix: Some((crate::KeyCode::Char('g'), crate::KeyModifiers::NONE)),
|
||||
..ViewState::default()
|
||||
};
|
||||
view.cursor_mut().selected = (3, 4);
|
||||
view.buffers.insert("item".into(), "East".into());
|
||||
view.expanded_cats.insert("Region".into());
|
||||
|
||||
let snapshot = Snapshot {
|
||||
view,
|
||||
seed: vec![
|
||||
Projection::Grid(GridUpdate::RowLabels(vec!["r0".into()])),
|
||||
Projection::Grid(GridUpdate::ColWidths(vec![8])),
|
||||
Projection::Grid(GridUpdate::Cells(vec![(
|
||||
(0, 0),
|
||||
CellDisplay::number("1,234.50"),
|
||||
)])),
|
||||
Projection::Formulas(vec!["Profit = Revenue - Cost".into()]),
|
||||
Projection::TileBar(vec![TileEntry {
|
||||
category: "Region".into(),
|
||||
axis: Axis::Page,
|
||||
}]),
|
||||
],
|
||||
};
|
||||
|
||||
let json = serde_json::to_string(&snapshot).expect("serializes");
|
||||
let back: Snapshot = serde_json::from_str(&json).expect("deserializes");
|
||||
assert_eq!(back, snapshot);
|
||||
}
|
||||
}
|
||||
|
||||
+14
-1
@@ -3,7 +3,9 @@ crate(improvise_formula,'crates/improvise-formula','formula parser, AST, parse_f
|
||||
crate(improvise_io,'crates/improvise-io','improv save/load, CSV/JSON import; no UI/commands').
|
||||
crate(improvise_command,'crates/improvise-command','command/effect layer + semantic App state; no ratatui').
|
||||
crate(improvise_main,src,'ratatui TUI rendering, draw loop, clap CLI').
|
||||
crate(improvise_protocol,'crates/improvise-protocol','wire vocabulary + portable session slice: Command, ViewState, RenderCache, reduce_view').
|
||||
crate(improvise_protocol,'crates/improvise-protocol','wire vocabulary + portable session slice: Command, ViewState, per-widget caches folded from Projection events, bootstrap-only Snapshot, reduce_view').
|
||||
crate(gp_tree,'crates/gp-tree','genetic-programming tree fitting; problem-agnostic, serde only. Owns the data module (preprocess, encode_row, read_csv) and the vendored SplitMix64.').
|
||||
crate(improvise_gp,'crates/improvise-gp','bridge: emits a fitted gp-tree Tree as an improvise formula string and validates it through parse_formula. Exists because gp-tree must stay problem-agnostic and improvise-formula must not drag GP into the wasm bundle.').
|
||||
layer(model,improvise_core).
|
||||
layer(view,improvise_core).
|
||||
layer(formula,improvise_formula).
|
||||
@@ -11,6 +13,8 @@ layer(io,improvise_io).
|
||||
layer(command,improvise_command).
|
||||
layer(rendering,improvise_main).
|
||||
layer(protocol,improvise_protocol).
|
||||
layer(fitting,gp_tree).
|
||||
layer(fitting_bridge,improvise_gp).
|
||||
location(add_keybinding,'crates/improvise-command/src/command/keymap.rs','default_keymaps()').
|
||||
location(add_command,'crates/improvise-command/src/command/cmd/registry.rs','default_registry()').
|
||||
location(add_effect,'crates/improvise-command/src/ui/effect.rs','impl Effect').
|
||||
@@ -94,6 +98,8 @@ doctrine(contract_test_for_the_unreachable_backend,'when one implementation of a
|
||||
doctrine(derive_the_enum_from_the_effects,'when a wire vocabulary must mirror an existing mutation set, enumerate the mutations and let that be the enum; inventing variants ahead of the mutations produces a protocol nobody can implement against').
|
||||
doctrine(force_the_failure,'an unchecked status whose failure yields a usable-looking value is not a style nit: force the failure and look at what the next line does with it. Four on one branch -- bd init exits non-zero but still leaves an empty database behind, an unchecked mv lands the new tracker INSIDE the old directory and reports success, jq status swallowed by a process substitution reads as \"no inputs found\" and exits 0, and a failed mktemp turns \"$TMP/$NAME\" into an absolute path at the filesystem root. Every one looked like success at runtime, so reading the failure paths found none of them; stubbing mv and mktemp to fail is what exposed them. Note chmod a-w proves nothing when the tests run as root -- it returned 0 and looked like a pass.').
|
||||
doctrine(kb_is_maintained_through_the_mcp,'prolog-sessions/kb.pl is the MCP serialized clause database, not a hand-written source file. Add facts by loadSession, then loadProgram, then saveSession -- never by appending text to the file. Note loadProgram REPLACES a whole predicate rather than appending to it, so the program you load must carry every existing clause of every predicate it touches, or the rest are silently dropped. Verify counts with findall/length before saving.').
|
||||
doctrine(fold_tests_must_test_order_not_the_axiom,'a test asserting fold(prefix) then fold(rest) == fold(whole) is VACUOUS: that identity is the definition of a left fold and holds for any apply() whatsoever. Proven 2026-09-19 by rewriting GridCache::apply to write every cell update to (0,0) -- both headline fold tests stayed green while the two real tests failed. Test the properties the design actually rests on: ORDER SENSITIVITY (which updates are dropped when they arrive early, i.e. that the fold is deliberately not commutative) and COMPACTION EQUIVALENCE (fold(compact(history)) == fold(history)), which is what \"seeded, then follows the event stream\" means in production. A seed taken as the literal head of the history presupposes the conclusion.').
|
||||
doctrine(a_macro_must_not_supply_a_forced_classification,'invariant(effect_kind_no_default) makes the compiler demand an EffectKind per effect precisely because a wrong one is silent. A macro that hard-codes the kind reintroduces the default under another name, for a category defined by SYNTAX (one field, one place) rather than SEMANTICS (per-client). Proven 2026-09-19: a simple_effect! instantiation writing the active view number format -- a textbook LayoutGeometry change -- compiled, reported ViewOnly, and passed all 294 command tests. If a macro covers a classified thing, make the classification a mandatory argument even when the answer is always the same.').
|
||||
work_done('improvise-8tk',bug_fix,'ZZ/:wq quit even when the save failed, silently discarding the model. Save now aborts the effect batch on failure; save_to_current_path/1 returns Result<String,String>; three tests in ui/effect.rs.').
|
||||
work_done('improvise-907',bug_fix,'Import wizard step machine redesign: validation_error() gate at ReviewProposals (no-category and over-limit), history-stack previous() bound to Left on every step, and Done made a real confirmation step holding the built workbook. Superseded improvise-gfq, improvise-o1c, improvise-c6j.').
|
||||
work_done('improvise-6sq',wont_do,'Merging EditOrDrill and EnterEditAtCursorCmd into one EnterEdit{mode} is not safe: they differ in WHEN the cell value is read, not in a mode parameter. Added ui/app.rs::tests::i_and_o_diverge_on_an_empty_records_grid as executable evidence.').
|
||||
@@ -161,6 +167,11 @@ forbidden(improvise_command,crossterm).
|
||||
forbidden(improvise_protocol,ui).
|
||||
forbidden(improvise_protocol,io).
|
||||
forbidden(improvise_protocol,commands).
|
||||
forbidden(gp_tree,ui).
|
||||
forbidden(gp_tree,commands).
|
||||
forbidden(improvise_gp,ui).
|
||||
forbidden(improvise_gp,io).
|
||||
forbidden(improvise_gp,commands).
|
||||
pipeline_step(1,keypress,keymap_lookup).
|
||||
pipeline_step(2,keymap_lookup,cmd_execute).
|
||||
pipeline_step(3,cmd_execute,effect_list).
|
||||
@@ -199,6 +210,8 @@ crate_depends_on(improvise_main,improvise_io).
|
||||
crate_depends_on(improvise_main,improvise_command).
|
||||
crate_depends_on(improvise_protocol,improvise_core).
|
||||
crate_depends_on(improvise_command,improvise_protocol).
|
||||
crate_depends_on(improvise_gp,gp_tree).
|
||||
crate_depends_on(improvise_gp,improvise_formula).
|
||||
depends_transitively(A,B) :-
|
||||
depends_via(A,B,[A]).
|
||||
depends_via(A,B,C) :-
|
||||
|
||||
Reference in New Issue
Block a user