diff --git a/context/code/core.md b/context/code/core.md index f5d5abf0..bda0a18b 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -94,7 +94,27 @@ target category and every `none_cat` stripped), and iterates `stem × formula target` until nothing changes, bounded by `MAX_EVAL_DEPTH = 16`. Chained formulas (`Margin = Profit / Revenue` over `Profit = Revenue - Cost`) converge naturally; circular references converge to `CellValue::Error("circular")` -rather than overflowing the stack. The stems are an `IndexSet`: dedupe is O(1) +rather than overflowing the stack. + +**A cell still moving when the bound runs out has not converged, and the cached +path had to be told so** (improvise-7iww). The direct evaluator gets this free: +`evaluate_depth` bottoms out at `depth == 0` and returns `Error("circular")`. +The fixed point had no such arm — it simply stopped iterating and left the +sixteenth iteration in `formula_cache`, which `evaluate_aggregated` then served +as a valid number. `Revenue = Revenue + 1` over a stored `Revenue = 10` read +`Error("circular")` directly and `Number(26)` through the cache, and the cached +one is the **displayed** path, because the default views put the virtual +categories on `Axis::None`. So a circular formula looked like a real calculated +value. `recompute_formulas` now records the keys that changed on each pass and, +if the last pass still changed any, writes `Error("circular")` over them. + +Note what this costs and why it is the right cost: the fixed point can only +*confirm* convergence with a pass that changes nothing, so a chain needing all +sixteen passes is now reported circular. That is the same bound the direct +evaluator already enforces — a sixteen-deep chain exhausts `depth` there too — +so the two paths agree rather than diverge, which is the property §3 exists to +protect. The anti-clamp guard is +`a_chained_formula_still_converges_after_the_circular_marker`. The stems are an `IndexSet`: dedupe is O(1) per cell and insertion order is preserved (improvise-6os replaced an O(n²) `Vec::contains`). @@ -541,7 +561,30 @@ fold routes through `eval_expr`, the cached arm is now reached **only by a bare The `Expr::Ref` fast path **stays**, and it is the same fold specialised: a record's `Ref` is that record's own cell, so summing them is summing the -matching values. It differs in exactly one place — a record with no such cell is +matching values. **Two of its premises were unstated and both were false** +(improvise-s08t, improvise-s2w3), which is worth recording as a shape: a +specialisation defended by an argument is only as good as the conditions the +argument assumed, and neither condition was being checked. + +*The name must resolve.* The narrowing was written `if let Expr::Ref(name) = +inner && let Some(cat) = find_item_category(name)`, so a name that resolved to +nothing left the partial key **unconstrained** — `SUM(Typo)` aggregated every +numeric measure matching the context and answered a plausible total. A missing +identifier now fails before the fast path is reached, with the same `ref:{name}` +the record-level evaluator uses. + +*The name must be stored.* A formula target is a `_Measure` item that nothing +wrote (improvise-nrs), so there are no raw cells behind it and the raw scan +returns nothing: `SUM(Double)` answered `0` where `SUM(Double + 0)` folded and +answered `20`. The premise of the specialisation — "a record's `Ref` is that +record's own cell" — is exactly what fails there, so the answer is to ask +whether it holds rather than to delete the path. `formula_for` is the one home +of "is this cell derived", so the check is that call and not a second +derivation; a derived ref falls into the fold, where `eval_expr`'s own `Ref` +arm routes through `evaluate_depth` and sees the formula. A target that also has +stored cells reads as the formula, because `formula_for` wins there too. + +It differs in exactly one place — a record with no such cell is *skipped* there and an **error** in the fold. That is deliberate: a bare `SUM(Revenue)` asks for the values that exist, while `SUM()` asks the expression of each member, and an expression that cannot be evaluated is an diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index a559e63a..aca67235 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2205,11 +2205,60 @@ exists would pass against a host that applied the first command and dropped the second — which is exactly what the previous rule did, loudly, and what the pre-frame check did, silently. -### a_frame_whose_later_mutation_fails_is_not_yet_undone +### a_frame_whose_later_mutation_fails_is_undone_whole -**Characterization of the open hole (clause 4), not an endorsement.** +**ADR 001 clause 4, delivered (improvise-yqpd). This entry keeps the hole it +characterized, because the inversion is only legible against it.** -The contract says a frame is all-or-nothing. It is not yet: `App::apply_effects` +The three assertions flipped. The removal is **undone**, the cell reads its +**pre-frame** value — `Some(Number(1.0))`, which is neither `5.0` (the frame's own +write landing) nor `None` (the removal surviving), and that middle value is the +whole difference between a rollback and an abort — and the frame is **refused out +loud** as `Rejection::ChainBroken`. A fourth assertion was added that the previous +version could not have: the **record counter** goes back too, or the client's next +batch is refused for a write that never happened. + +**The mechanism, and where it deviates from the ticket.** The ticket asked for +snapshot-and-restore at `App::apply_effects`. What landed is `App::snapshot_document` +/ `App::restore_document` — the workbook, its dirty marker and the two counted tiers, +the record tier riding inside the workbook's `DataStore` — driven from +`Host::commands`, which takes the snapshot before the frame and restores it on +failure. It is **not** taken inside `apply_effects` itself, and the reason is cost: +`apply_effects` runs on every keypress, and an unconditional `Workbook` clone there +would put a full document copy on the held-key path that gotcha 24's `mcq4` work +exists to keep cheap. The primitive is on `App`, so a future `RunScript` opens the +same transaction; only the *trigger* is at the frame. + +**What counts as a failure, and the split the ticket did not name.** Two things: +an effect that sets `abort_effects` (`Rejection::MutationRefused`), and a command +whose record no longer resolves (`Rejection::ChainBroken`). The second is the +interesting one. The ticket says interleaving the record check with the applies +would close clause 4's silence for free but would sell the legitimate double-write — +a frame writing one record twice works only because the second write is compared to +the record's *pre-frame* counter. That is true of the **counter** half of the record +tier and **not** of the **structural** half: writing a record twice cannot make it +stop resolving. So the record tier splits — *does it resolve* is asked again before +every mutation (`reduce::broken_chain`), *is its counter what you claimed* stays +pre-frame — and clause 4 gets its loud failure without paying the price the ticket +costed. `a_later_write_in_a_chain_still_answers_for_its_own_record` arm 3 still +passes, which is the guard that the trade was not taken by accident. + +**Injected faults, each watched to fail.** Making `undo_frame` restore nothing +fails this test alone. Making `broken_chain` return `None` unconditionally fails +this test and the mirror test. What did **not** discriminate: removing the +frame-creates-it exemption from `broken_chain` fails nothing, because +`App::resolve_record` falls back to `record_in_layout`, so a record whose item an +earlier command in the frame added already resolves through the layout before any +cell exists. The exemption is therefore correctness-preserving and **not currently +observable**; it is written as the *same* predicate the pre-frame check exempts by +(`will_be_created_by_the_frame`) rather than a second copy, so the two cannot drift +even though no test holds them together. + +--- + +The hole this replaced, kept because the inversion is only legible against it: + +The contract says a frame is all-or-nothing. It was not: `App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a rollback), so a chain whose later mutation fails leaves the earlier one standing. `[RemoveItem("North"), SetCell(the North record)]` is the shape: the removal lands, @@ -2231,12 +2280,79 @@ It is unreachable from any client here — the only producer of a multi-command is `keys::commit`, which emits one mutation among view commands — which is what makes it safe to document rather than rush. -**This test must be inverted when clause 4 lands**: all three assertions flip, -because then nothing at all happens. - The frame claims North at its current counter, so it passes the check and fails at *apply* time — which is the only way to reach the hole. A frame refused by the check -never applies anything and would be testing clause 2 instead. +never applies anything and would be testing clause 2 instead. That fixture note +survives the inversion unchanged: it is still the only route to the arm. + +### the_mirror_settles_its_mode_against_every_grid_it_is_later_projected + +improvise-el9h. `ClientSession::fold` settled `view.mode` against `self.grid` — the +grid that session was **last projected** — and `project_diff` then assigned +`self.grid = desired` and never settled again. The client does not have that shape: +`ClientState::fold` calls `settle_mode()` on every fold including +`Command::Project`, so when the new view's grid arrives the client re-settles and +the mirror does not. + +`[SwitchView(records-shaped view), ChangeMode(Normal)]` is the shape — precisely the +case PR #98's settle exists to serve. **Demonstrated before the fix**: the client +ended on `RecordsNormal` and the mirror on `Normal`. The fix is `settle_mode` called +from `fold` *and* at the end of `project_diff` once the grid has moved, which also +closes the three bypass doors (`apply_switch`, `adopt_deleted_views`, +`adopt_cursor_epoch` write the mirror with bare `reduce_view`): whatever they leave, +the next projection settles. + +**Two preconditions, and one of them caught a broken fixture on its first run.** The +test asserts the client starts on `Normal`, so the settle has somewhere to move the +mode *from*. And `app_with_a_records_shaped_second_view` asserts the second view +really reaches records mode — which failed immediately, because the view was built +with `View::new` and a fresh `View` carries no axis entry for `_Index`/`_Dim`, so +`is_records_mode` can never fire on one. Without that assertion the test would have +compared two modes that were both `Normal` for a reason having nothing to do with +the bug, and passed against the unfixed code. + +The yardstick is a real `improvise_protocol::ClientState` fed the same projections +in arrival order, not a hand-written expectation: the property is that the mirror +and the client run the same function over the same stream. + +### a_failed_frame_puts_the_askers_mirror_back_on_the_view_it_was_on + +Clause 4's least obvious cost, in the ticket's own words: **rolling back the `App` +is not enough.** `Host::command` projects per command and `apply_switch` / +`adopt_deleted_views` / `adopt_cursor_epoch` move mirrors inline, so by the time a +later command fails, every session's mirror has already moved. `Host::commands` +therefore snapshots the whole `Sessions` beside the document and restores both. + +**The first version of this test was vacuous, and the injection is what showed it.** +It seated a bystander on a second view and asserted the bystander was told nothing +and its mirror was unmoved — both of which hold when `undo_frame` restores *nothing +at all*, because the frame's projections are discarded from `out` either way and +nothing ever moved that bystander. It answered *did the frame return early* where it +meant *was the mirror put back*. + +The rewrite aims at the one mirror move a failed frame really makes and that +**nothing in the stream can ever correct**: the asker's own `SwitchView`. An +accepted switch is relayed to nobody (gotcha 36), so a mirror left on the switched-to +view diverges from a client that was told the frame was refused, permanently and +silently. The fixture asserts up front that the mirror starts on `Default` — the +direction the default cannot supply — so the closing assertion is not satisfied by a +mirror nothing ever wrote. Removing `self.sessions = mirrors` while keeping the +document restore fails this test and only this test. + +### a_frame_that_creates_the_record_it_writes_is_not_a_broken_chain + +The other direction: clause 4 must not fire on `[AddItem("West"), SetCell(West)]`, +which is the exemption ADR 001 states as a **property** — a record that neither +resolves nor is claimed is one the frame is creating. + +**Be clear about what this does and does not catch.** It catches `broken_chain` +being asked against the *pre-frame* document instead of the running one, which is +the plausible implementation error (passing the snapshot where the live `App` +belongs) and would refuse every data-dependent chain. It does **not** catch the +exemption filter being dropped, for the reason recorded above: `resolve_record` +falls back to the layout, so `West` already resolves once `AddItem` has run. A +fixture that discriminated the filter would need a created record that never enters +the layout; none exists in this workspace and one was not invented for it. #### Helper — `frame_basis` diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index eb9d8a53..509e93ae 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -12,6 +12,107 @@ none. ## `model/types.rs` +### a_chained_formula_still_converges_after_the_circular_marker + +The anti-clamp half of improvise-7iww. `recompute_formulas` now publishes +`Error("circular")` for any cell still moving after `MAX_EVAL_DEPTH` passes, and +a guard that refuses everything would satisfy the reproduction just as well as +the fix does. This test runs a three-deep chain — `Cost`, `Profit`, `Margin` — +that converges in two passes and asserts the *numbers*, so a marker that fired +on a converging chain fails here. Inject the fault by marking every cell +evaluated in the last pass rather than only the ones that changed. + +### a_derived_measure_with_stored_cells_does_not_leak_the_raw_value + +improvise-s2w3's stated edge case. The fix routes a bare `SUM(Double)` through +the per-record fold when `Double` is a formula target, and the interesting case +is a target that *also* has stored cells: `formula_for` is the one home of "is +this cell derived", so the fold reads `20` (the formula) and not `999` (the raw +cell). Fails if the fast path is reinstated for a derived name, or if the fold +were built to prefer raw data over `formula_for`. + +### an_aggregate_over_a_derived_measure_sees_the_formula_in_both_evaluators + +improvise-s2w3, widened past the reproduction. `eval_agg` specialised +`Expr::Ref` into a raw `DataStore` lookup; a formula target has no raw cell +behind it (improvise-nrs), so `SUM(Double)` summed nothing and answered `0` +while `SUM(Double + 0)` folded per record and answered `20`. core.md §13 +defends that fast path and the defence still holds — it is the fold specialised +to a ref naming a *stored* cell. What failed is the premise, not the +specialisation, so the fix asks `formula_for` rather than deleting the path. + +The test runs `SUM`/`AVG`/`MIN`/`MAX` and both evaluators (direct `evaluate`, +and `evaluate_aggregated(OverHidden)` after `recompute_formulas`), because §3 +says a rule stated in one evaluator and not the other is a live bug. `COUNT` is +excluded deliberately: `COUNT(Double)` and `COUNT(Double + 0)` both count +members and agreed even before the fix, so including it would have added a +passing case that discriminates nothing. Inject the fault by hard-coding +`derived = false` in `eval_agg`. + +### an_unknown_name_inside_an_aggregate_errors_for_every_function + +improvise-s08t. `eval_agg` narrowed its partial key only `if let Some(cat) = +find_item_category(name)`; when the lookup failed the key was left +**unconstrained**, so `SUM(Typo)` aggregated every numeric measure matching the +context and returned a plausible wrong total. A typo is silent and looks like a +number. The test asserts the error *text* (`ref:Typo`) across all five aggregate +functions and both evaluators, so a fix that errors with some other message — or +that errors only where the raw scan happened to be empty — still fails. Inject +the fault by restoring `unwrap_or("_Measure")` in place of the `ok_or_else`. + +### both_evaluators_answer_every_comparison_the_same_way + +improvise-vyrx's *behavioural* half, and it is deliberately the smaller half. +The ticket's defect is that `eval_bool_cached` wildcarded `BinOp` where +`eval_bool` listed its arms, so a new variant would error at runtime in one +evaluator and fail to compile in the other — an asymmetry no runtime test can +see today, because both paths answer `Err("type")` for every arithmetic +operator. **The guard is the compiler**: with the wildcard removed, adding a +`BinOp::Mod` fails the build at four sites (`eval_expr`, `eval_bool`, +`eval_expr_cached`, `eval_bool_cached`) where it previously failed at three, and +that was verified by adding the variant and reading the errors. + +**A sweep for the shape found a second instance one level up**, in the same two +functions: both `eval_bool` and `eval_bool_cached` also wildcarded the outer +`Expr` match as `_ => Err("type")`, so a new `Expr` variant that ought to be +usable as a condition — an `And`, say — would have become a silent type error in +*both*. That is not vyrx's asymmetry (the two agreed), but it is vyrx's defect: +a new variant answered wrongly with nothing to notice. Both now list the six +non-comparison variants, and adding an `Expr::And` fails the build at four sites +where it previously failed at two. Verified the same way. + +**What the sweep also settled is which wildcards to leave**, which is the more +useful half. `eval_agg`'s own `match inner { Expr::Ref(..) => …, _ => false }` +is deliberately *not* exhaustive: it falls through to the **general** path — the +per-record fold — which is the right answer for any expression that is not a +bare ref, including one that does not exist yet. The dangerous wildcard is the +one that falls through to a **refusal** or to a **specialisation**; a wildcard +that falls through to the general case is safe by construction. The same reading +clears `CellValue::as_f64`'s `_ => None` and `CategoryKind::of_name`'s +`_ => Regular`, whose inputs are a value that is not a number and a name that is +not reserved. + +What this test adds is the neighbouring property the exhaustive arms do not +give: that the six comparison arms *agree in value* between the two evaluators. +It runs each operator against a true and a false right-hand side, so an arm +mistyped in one path (`Lt => lv <= rv`) fails. Both directions matter — a single +right-hand side is satisfied by an evaluator that answers a constant. + +### re_adding_an_existing_category_at_the_limit_creates_nothing_and_succeeds + +improvise-f86q. `Model::add_category` checked `MAX_CATEGORIES` *before* +`contains_key`, so at twelve regular categories re-adding an existing name was +refused with "Maximum of 12 categories reached" — a false statement, since the +category exists and nothing would be created. Per gotcha 21 `effect::AddCategory` +sets `abort_effects` on refusal, so typing an existing name into the records-mode +`+cat` column at the limit aborted the rest of the batch: the improvise-2zq +failure mode by another route. + +The fix is a hoist. The test asserts three things, and the third is what stops it +from being satisfied by deleting the limit: the returned id is the existing +category's, the category count did not move, and `add_category("C12")` — a name +that really would create something — is still refused. + ### a_hidden_dimension_list_reduces_to_one_of_two_states improvise-5jm5. `evaluate_aggregated` took `none_cats: &[String]` and consulted @@ -42,6 +143,15 @@ difference is visible. improvise-kdp named both after they had been duplicated inline in two evaluators; the test pins the contract directly rather than only through formula eval. +Its **vehicle changed** with improvise-s08t. It reached "no rows" through +`Expr::Ref("Nonexistent")` — a name that resolves to nothing — which was +precisely the bug s08t names: an unresolvable ref left the selection +unconstrained instead of erroring. So the test was standing on the defect to +demonstrate a neighbouring property. It now uses a name that **resolves** and +has no cells (`Revenue`, declared and never written), which is the emptiness +the entry is about; an unknown name is now an error and is pinned separately by +`an_unknown_name_inside_an_aggregate_errors_for_every_function`. + ### find_item_category_falls_back_to_formula_targets `add_formula` never inserts its target into `_Measure` (the item is *implied* by @@ -164,6 +274,29 @@ that the display really does round (so the fixture is discriminating) and that `Total = Price + Tax` is `10.75` and not `11`. Without the first assertion the test would pass on a fixture where rounding changes nothing. +## `format.rs` + +### a_finite_number_never_displays_as_infinity + +improvise-pikk. `round_half_away` scales by `10^decimals` before truncating, so +`n * factor` overflows to infinity for a large finite `n` and any nonzero +precision: `format_f64(1e307, false, 2)` rendered `"inf"`. The model held the +number correctly and the display invented an infinity. Gotcha 8 is why nothing +else caught it — `format_f64` is the whole display path and never feeds back +into eval, so this is a wrong pixel with no wrong number behind it to trip a +formula test. + +Reachability is low and that is not a reason to leave it: the shipped default +format is `",.0"`, whose factor is `1`, so it needs both an absurd magnitude and +an explicit `.N` format. + +The test carries its own anti-clamp assertion, because "return `n` unchanged" +satisfies the overflow case *and* every other case. `1e307 * 100` overflows and +`1e306 * 100` does not, which pins the threshold; and `format_f64(2.5, false, 0) +== "3"` pins that the rounding path is still reached at all — half-away-from- +zero, where a passthrough would hand the value to `{:.0}` and get Rust's +half-to-even `"2"`. Inject the fault by dropping the `is_finite` arm. + ## `model/cell.rs` ### matching_cells_yields_the_primary_insertion_order @@ -351,6 +484,41 @@ test flips the category back to `Page` afterwards, so it distinguishes "omits The fixture puts a category on each of `Page`, `Filter`, `Row` and `None` so the result separates the two questions rather than merely counting. +### on_category_removed_forgets_a_collapsed_group + +improvise-qx1z arm 2. `View::on_category_removed` cleared `category_axes`, +`page_selections` and `hidden_items` — but not `collapsed_groups`. The other +three lines being right there is what makes this an omission rather than a +decision: collapse `Month/Q1`, delete the category, re-add it with `Jan[Q1]`, +and the group comes back **pre-collapsed** with nothing saying why. Reachable +from shipped keys, through the category panel. + +Fault injected: delete the added line; the test fails at +`is_group_collapsed("Month", "Q1")`. The test re-adds the category after +removing it, because a view that merely *forgot the axis* would also report +`false` for a category it no longer knows — asking the question after the +re-add is what separates "cleared" from "not currently registered". + +### on_item_removed_forgets_a_hidden_item + +improvise-qx1z arm 1, at the `View` level. `Model::remove_item` removes the item +and its cells and **touches no view at all**, so `hidden_items["Region"]` kept +`"East"`: re-adding the item put it in the model and in the category panel while +leaving it absent from the grid, with no indication why. + +The fixture hides **two** items and removes one. Without the second, a fix that +dropped the whole category entry — `hidden_items.remove(cat_name)` — would pass +while destroying unrelated state; the surviving `West` assertion is the input +that separates "forget this item" from "forget this category". + +### on_item_removed_forgets_a_page_selection_naming_it + +The third question improvise-qx1z asked to check while there, and the answer was +the same: a page selection naming a removed item survived it. Asserted in both +directions in one test — removing a *different* item must leave the selection +alone — because a hook that cleared the selection unconditionally would satisfy +the removal half and silently reset the user's page on every unrelated delete. + ## `view/layout.rs` ### prune_axis_items_indexes_data_positions_not_visual_ones @@ -612,6 +780,96 @@ Fault injected by deleting either emission: the corresponding test fails with `Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and `the_two_front_ends_draw_the_same_column_header` fails with it. +### a_collapsed_groups_header_survives_pruning + +### a_header_whose_items_all_prune_away_is_dropped + +### an_ungrouped_item_after_a_pruned_group_belongs_to_no_group + +improvise-lhi4 and improvise-u6gc: two defects in one `match` block, and they +pull in opposite directions, which is why they were fixed together. The naive +repair of the first — stop pruning headers — deepens the second. + +`prune_axis_items` kept a header only if a data item under it survived the +predicate, and dropped a `GroupEnd` whenever the header it closed was still +pending. Both readings are wrong about a case the entry stream can distinguish. + +**lhi4**: a collapsed group contributes **zero** `DataItem`s — `expand_category` +emits the `GroupHeader` and then `continue`s past every item — so its header +was always pruned and the group vanished from the grid entirely. `z` +(toggle-group-under-cursor) resolves the group from `row_group_for(cursor)`, +so with no header and no items no row named it: collapse became one-way from +the grid, recoverable only through `:toggle-group `. + +**u6gc**: dropping a pruned header's `GroupEnd` **re-opens an earlier run**. +With `Jan[Q1], Feb[Q1], Mar[Q2], Sep` and `Mar` empty, the kept list became +`[H(Q1), Jan, Feb, Sep]` and the nearest-preceding walk answered `Q1` for +`Sep` — exactly the inheritance improvise-gd83 added `GroupEnd` to prevent, so +the regression was live whenever `prune_empty` was on. + +The fix distinguishes the two emptinesses **without consulting the `View`**, +which `GridLayout` has already dropped: a run that contained no `DataItem` at +all was never expanded, i.e. collapsed, and its header survives; a run that +contained items the predicate rejected is genuinely empty and goes. A +`GroupEnd` is kept whenever a run is open in the output, so a re-opened earlier +header is closed even though the header between them was pruned. That keeps +gotcha 1's shape — still a predicate over the data index, no `&View` +parameter, no flag vector. + +The three tests are separate because each is satisfied by a fix that misses +another. `a_header_whose_items_all_prune_away_is_dropped` **passes before the +change** and is the discriminator: it is what stops "keep every header" from +counting as a fix for lhi4, and it is the acceptance criterion lhi4 names. + +Faults injected, each against the fixed code, and each fails a **different** +pair — which is the evidence that no one of the three tests stands in for the +others: +- stop keeping the `GroupEnd` (drop the `run_open` push): both membership + tests fail with `Some(("Month", "Q1"))` against `None`; + `a_header_whose_items_all_prune_away_is_dropped` still passes. +- drop the `!had_item` condition in `close_pending_run`, keeping every pending + header: only `a_header_whose_items_all_prune_away_is_dropped` fails. +- invert it to `had_item`, keeping only headers whose items all pruned: + `a_collapsed_groups_header_survives_pruning` and + `a_header_whose_items_all_prune_away_is_dropped` fail, and the `GroupEnd` + test still passes. + +Both fixtures fund their cells explicitly, so `prune_empty` really prunes: on +an unfunded fixture every row prunes and the tests would assert over an empty +grid. `row_count()` is asserted first in each for that reason. + +### wide_cell_clamps_without_overflow + +improvise-yae4. `grid_body` narrowed each measured width to `u16` and only then +added the one-character gap and clamped to `MAX_COL_WIDTH`, so the arithmetic +ran at full width before the cap that makes it safe. A stored 65535-column text +value panicked at `w + 1` in debug; in release the sum wrapped to `0` and the +column was drawn at `MIN_COL_WIDTH` — the *narrowest* column, for the widest +value. A 65536-column value never reached the addition at all: `as u16` +truncated it to `0` first, so it wrapped in debug builds too, silently. + +Widths are now capped while still `usize`, in the one place every measurement +enters (`widen`), which is also why `header_level_seed` folds through it rather +than keeping a second `as u16`. The answer is unchanged for every input that +did not overflow: `(min(w, 32) + 1).clamp(5, 32)` and `(w + 1).clamp(5, 32)` +agree everywhere, because both saturate at 32 for any `w >= 31`. + +`improvise_protocol::header_level_widths` carried the identical defect on the +row-header axis — a 65535-column *item label* — and is fixed the same way. It +is the one home for that rule (gotcha 44), so the row header and the column +widths do not get two answers. + +The test asserts `widths == vec![expect]`, not `all(<= 32)`: the length pins +that the fixture really produces **one** column, and the ordinary 20-column arm +pins that the single column is the one carrying the value — a fixture in which +no column was wide would read `MIN_COL_WIDTH` there and pass a `<= 32` check. + +Faults injected, each against the fixed code: +- `width as u16` in `widen`, debug: the 65535 arm panics with + `attempt to add with overflow`. +- the same, release: the 65536 arm fails with `left: [5]`, `right: [32]` — + the wrap reported as the minimum width. + ### a_trailing_conversion_letter_does_not_erase_the_precision improvise-711n. `parse_number_format` read the precision as diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 4fe396c3..5f04b7ef 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -15,6 +15,55 @@ helper under `###` would pass today and fail the moment that is tightened. ## `improvise-formula/src/parser.rs` +### a_comparison_the_walker_does_not_know_is_not_ge + +The `cmp_op` half of improvise-492x. `parse_cmp_op` ended `_ => BinOp::Ge`, so +a `<>` alternative added to `cmp_op` in `formula.pest` would have parsed as +`>=`: a filter that silently means something else. Same shape and same fix as +the aggregate half above; the token `<>` is chosen because it is the spelling a +future grammar would most plausibly add. + +### an_aggregate_the_walker_does_not_know_is_not_count + +improvise-492x, one of three beads that are the same defect — with +improvise-vyrx (`eval_bool_cached` wildcarding `BinOp` where `eval_bool` lists +it) and improvise-4lgj (the `.improv` pest walker's `_ => {}` arms silently +skipping unhandled rules). design-principles §2 states the rule they all break: +prefer an exhaustive match, so that adding a variant produces a compile error +that names every site needing an update. Each of the three is a place where a +wildcard turned that compile error into a **silent** wrong answer. + +Here the wildcard was `_ => AggFunc::Count`. Adding `| ^"MEDIAN"` to `agg_func` +in `formula.pest` would have made `MEDIAN(Revenue)` parse and evaluate as +`COUNT(Revenue)` — a plausible number, no error, nothing to notice. + +**A compile error is not available and that is a property of the input, not a +choice.** `agg_func` is one pest rule matching five string alternatives, so +`pest_derive` generates no per-alternative `Rule` variant for Rust to match +exhaustively over; the walker's input is a `&str`. So the refusal is the next +best thing and it is the idiom this file already uses for exactly this +condition: `GRAMMAR_INVARIANT`, whose text is "parser out of sync with +formula.pest". The module's own header comment — "tree walkers: infallible, +because pest validated the pair first" — is what makes a panic here the honest +shape rather than a `Result` threaded through every walker for a state no user +input can reach. + +The test injects the ticket's own fault: it calls `parse_agg_func("MEDIAN")` +directly and requires a panic. On the pre-fix code that call returns +`AggFunc::Count` and the test fails. Its companion, +`every_grammar_aggregate_and_comparison_is_spelled_out`, is the anti-clamp +direction — a walker that panicked on *everything* would satisfy the two +`should_panic` tests, so the five aggregates and six comparisons are asserted to +still map to what the grammar says they mean. + +### every_grammar_aggregate_and_comparison_is_spelled_out + +The anti-clamp half of improvise-492x — see +`an_aggregate_the_walker_does_not_know_is_not_count`. It also stands as the +list that must be edited when `formula.pest` gains an alternative: the walker +refuses the new token, and this test is where the accepted set is written down +in Rust rather than only in the grammar. + ### parse_sum_with_inline_where_filter Regression: a `WHERE` inside the aggregate's parens must parse as the @@ -442,6 +491,118 @@ honest — without it, "the writer quotes it" would be consistent with the reade also accepting the unquoted form. The trailing precondition (the same line quoted *does* parse) stops it passing because of some unrelated syntax error. +### prune_empty_survives_the_roundtrip + +improvise-qu4a. `View::prune_empty` was never written to disk: `format.rs` wrote +`format:`, `hidden:` and `collapsed:` for a view and nothing else, and +`improv.pest`'s `view_entry` had no `prune` alternative. Pressing `P`, saving +and reopening brought every pruned row and column back — and the toggle marks +the document dirty, so the user is invited to save the setting that is about to +be discarded. + +The reason nothing caught it is the shape worth remembering: it is **invisible +to `fmt(parse(x)) == fmt(parse(fmt(parse(x))))`, because both sides drop it** +(`doctrine(a_roundtrip_equality_is_blind_to_a_loss_both_sides_share)`). So this +test asserts the **flag**, never the output's stability. Fault injected: delete +the two-line `w!` in `format_md`; the test fails at the `prune: true` assertion, +and the two controls below still pass — which is exactly why they are controls +and not the evidence. + +#### a_view_that_does_not_prune_writes_no_prune_line + +The control, stated separately because it is the direction the default supplies. +A writer that emitted `prune: true` unconditionally passes the test above and +fails this one. Not evidence of the bug; evidence the fix is not one-directional. + +#### a_file_without_a_prune_line_does_not_prune + +The compatibility reading, decided rather than defaulted: a file written before +the line existed means `false`, which is what happens today. `improv.pest` +spells `prune: true|false` rather than making the line's presence the boolean, +so the grammar-walking generator can emit `prune: false` and it round-trips to +`false` through a writer that omits it. + +### a_file_naming_its_own_views_gains_no_default + +improvise-m4rv. `Workbook::new` seeds a `Default` view and `on_category_added` +assigns every parsed category into **every** existing view, so a file naming +only `Plan` parsed into *two* views — the second auto-laid-out and never in the +file. Two consequences, the second worse: the shipped demo's view panel showed a +phantom row **at index 0**, so the first `V`+`Enter` a visitor pressed took them +off the curated view; and open-and-save with no edits rewrote the user's +document, appending the whole `## View: Default` block permanently. + +`examples/demo.improv` was unaffected only because its view *is* named +`Default`, which is why the bug survived the shipped-document tests. + +Fault injected: remove the `drop_unnamed_default` call in `build_workbook`; the +test fails with `["Default", "Plan"]`. The fix is narrow on purpose — it drops +`Default` only when the file named **at least one** view and none of them was +`Default`, so the two cases in the control below are untouched. + +#### a_file_naming_no_view_keeps_the_seeded_default + +The control the fix must not break, in both directions: a file with no `## View` +section still opens on the seeded view (the legacy reading), and a file that +names `Default` itself keeps it. Without this, "drop Default" could have been +implemented as "always drop Default" and the test above would not have noticed. + +### the_aggregation_section_is_written_in_a_stable_order + +improvise-ppru's companion. `Model::measure_agg` is a `HashMap`, and an index +whose iteration order is hash order is not an order (gotcha 18) — writing it +unsorted would give a different `.improv` file per process, the same class of +defect as `analyze_records`. The test pins the three lines in name order and +then asserts `format(parse(text)) == text`. Fault injected: drop the +`sort_by_key`; the assertion fails on most runs, which is itself the argument +for pinning it rather than trusting a single green run. + +### an_interleaved_category_keeps_its_item_order + +improvise-3yv3. `format_md` collected every ungrouped item into `bare` and +emitted that whole list *before* any grouped item, so a category built as +`[grouped, ungrouped]` came back `[ungrouped, grouped]`. Grid axis ordering +consumes item insertion order (gotcha 18), so reopening a document silently +rearranged its rows and columns. + +The fixture interleaves deliberately — `a, b, g1[G], c, g2[G]` — because the +ticket's own two-item reproduction cannot tell "writes grouped items last" from +"writes each run in place": with one item of each kind, *both* orders are a +single bare run followed by a single grouped one. The test asserts the four +emitted lines, not just the parsed order, so a writer that happened to re-sort +correctly by accident still fails. Fault injected: restore the two-vector split; +the line assertion fails with `["- a, b, c", "- g1[G]", "- g2[G]"]`. + +### a_cell_with_no_coordinates_round_trips + +improvise-pnf5, the P1. A cell keyed by the empty `CellKey` is a legal model +state — it is what `AddRecordRow` leaves while a records row is unfinished, and +it is reachable from the shipped `r` then `o` keys. `format_md` wrote it as +`" = 0"` and `coord_list` required at least one coordinate, so **saving in +records mode overwrote a good document with one the application could not +reopen**. `CleanEmptyRecords` runs on leaving records mode, not before `Save`. + +The fix is a spelling rather than a discard, because the ticket is explicit that +legitimate data must not be dropped: `data_coords = _{ (coord_list ~ " ")? }` +gives the scalar cell the line `= 42`. It is unambiguous under PEG because +`coord` needs a `name`, and `bare_name` cannot start with `=`, so the optional +group fails cleanly and the `"= "` literal matches. + +Fault injected: revert the writer's `match` to the old `"{} = {}"`; the test +fails at the `\n= 42\n` assertion, and `parse_md` then rejects its own writer's +output. The trailing `format_md(&back) == text` is what makes this a round trip +rather than a parse check. + +#### slash_in_hidden_name_survives_roundtrip + +**Not a bug.** improvise-nvq4 (external report) claimed a `/` in an item or +group name corrupts the `hidden:`/`collapsed:` line, whose format is +`Cat/Item`. It does not reproduce: `hidden_line` reads two `name`s, a `name` is +`pipe_quoted | bare_name`, and `bare_name` excludes `/` — so `quote_name` +already writes `hidden: C/|A/B|` and `extract_name_pair` reads it back. The test +is kept as a **passing control** over both maps, so the claim cannot be re-filed +without someone first watching this pass. + --- ## `improvise-io/src/persistence/export.rs` @@ -520,9 +681,65 @@ whenever the eleventh-and-later values are the ones that rule a format out. Twelve values are ambiguous and one (`25/12/2025`) is day-first only. In hash order it landed inside the first ten about half the time, so the column imported as `%d/%m/%Y` on some runs and `%m/%d/%Y` on others — every date in it off by the -difference. In first-seen order it is last, deliberately: the answer is -`%m/%d/%Y` **stably**, which is the property asserted. The ten-sample truncation -itself is unchanged and is still a real limit. +difference. In first-seen order it is last, deliberately, and the answer is +**stable**, which is the property asserted. + +**The expected value moved with improvise-jcfs, and the reason is worth keeping.** +This entry used to say the answer was `%m/%d/%Y` and that the ten-sample +truncation "is still a real limit". That is exactly backwards as a test: the +fixture was built so the disambiguating value sits *outside* the window, so the +assertion was pinning the truncation as correct behaviour — a test written to +prove determinism had, as a side effect, made the neighbouring defect a +specification. With the window gone the twelfth value is seen and the answer is +`%d/%m/%Y`. Nothing about hash-seed independence changed; only the constant did. +The general form: **a fixture chosen to exercise bug A can silently enshrine +bug B**, and the tell is that the expected value is surprising for a reason the +entry does not explain. + +### a_disambiguating_date_after_the_eleventh_value_still_decides + +improvise-jcfs. `detect_date_format` took `samples.into_iter().take(10)`, so a +date that can only be read day-first was never examined if it appeared after the +tenth *distinct* value. `%m/%d/%Y` precedes `%d/%m/%Y` in `DATE_FORMATS`, so the +column silently imported month-first and every derived `_Month`/`_Quarter` value +was wrong. The cap is removed; `all()` short-circuits on the first failure, so a +format that does not fit costs one parse either way. + +**Fault injected:** `take(10)` restored. The test fails, as does +`the_detected_date_format_does_not_depend_on_the_hash_seed`. The eleven leading +values are asserted to parse under *both* readings first, so this is a statement +about a value outside the old window deciding, not about one format merely +fitting — the same precondition `an_ambiguous_date_is_read_american_first` uses. + +### a_blank_value_does_not_demote_a_numeric_column + +improvise-l1f5. `all_numeric` was `values.iter().all(|v| v.is_number())` over +*every* value including `Value::Null`, and `parse_csv_field` maps a blank CSV +field to `Null`. So one blank cell in a thousand-row Revenue column proposed +`Label` instead of `Measure`, and on a default import the numbers then arrived as +**category item names** rather than as cell data. Inference now runs over the +values that are present. + +**Fault injected:** both `all_` bindings restored to the full `values` slice. +This test and its string sibling fail; `an_all_null_column_is_a_label` does not, +which is correct — it is the control. + +### a_blank_value_does_not_demote_a_string_column + +The same defect one branch over, and the reason the fix touched two lines rather +than one. `all_string` had the identical shape, so a Region column with one blank +became a `Label`: not merely a wrong proposal, but a column whose +`distinct_values` were then computed over a slice the branch had already +rejected. Fixing only the numeric half would have left a bug with the same cause, +the same shape and no entry here. + +### an_all_null_column_is_a_label + +Stated because `all()` is vacuously true over an empty slice: with the `Null`s +filtered out, a column that is entirely blank would satisfy `all_numeric` *and* +`all_string` and be proposed `Measure` on the strength of no evidence at all. +The `!present.is_empty()` guard is what makes it fall through to `Label`, and +this test is the only thing that names that as a decision. --- @@ -585,6 +802,84 @@ The degenerate shape for everything downstream that splits a row into coordinates and a measure: one field, present, and **not** mistaken for a short row. +### a_nonfinite_numeric_token_stays_text_rather_than_becoming_zero + +improvise-jc95, rated P1. `serde_json::Number::from_f64` returns `None` for NaN +and the infinities — JSON has no spelling for them — and `parse_csv_field` +answered that `None` with `.unwrap_or(serde_json::Number::from(0))`. So `NaN`, +`inf`, `-inf` and any overflowing literal such as `1e999` imported as a **real +measured zero**: not a blank, not an error, a number that sums and averages and +is indistinguishable from data. The token now falls through to the existing +string arm, which is what an unparseable field already does. + +**Fault injected:** the `unwrap_or` restored. The test fails on the first token. +The precondition inside the loop asserts each token really does parse as a +non-finite `f64`, so a future `f64` parser that rejected `inf` outright would +fail the precondition rather than pass the test vacuously. + +The consequence downstream is deliberate and worth stating: a column carrying +`NaN` is no longer all-numeric, so the analyzer proposes it as a Category or +Label rather than a Measure. That is the honest answer — the column is not +numeric — and it is visible in the wizard, where a fabricated zero was not. + +### an_ordinary_float_is_still_a_number + +The control for the entry above. Guarding non-finiteness by deleting the float +branch entirely would also pass that test, so this one pins that `1000.50` is +still a `Number` and that `0` — the value the bug invented — is still reachable +when the file really says so. + +### a_repeated_header_is_renamed_rather_than_overwriting_its_twin + +improvise-5hfh. Headers key a `serde_json::Map`, so `A,B,A` inserted the third +column's value over the first column's under one key: the record kept two fields +where the file had three, with no error and no warning, and the analyzer then +saw a column that did not exist in the source. Later occurrences are now renamed +`A_2`, `A_3`, … and each rename is reported. + +**Fault injected:** `disambiguate_headers` short-circuited to `(raw, vec![])`. +The test fails on `map["A_2"]` being absent, and so does +`duplicate_csv_headers_not_silent`. + +The warning names the original, the 1-based column and the new name, because a +user's next question is which of two identically-named columns became `A_2`. +Note the enabled reproduction asserts the **observable** outcome first — three +fields, values `1`, `2`, `3` in order — and only then that a warning mentions +it; its original form asserted prose alone, against a module that emitted no +warnings at all and so had no wording convention to match. + +### a_rename_skips_a_suffix_the_file_already_claims + +The repair's own failure mode. `A,A_2,A` renaming the third column to `A_2` +would collide with the real second column and reintroduce exactly the overwrite +the fix exists to prevent — a bug strictly worse than the original, because the +warning would say the data was preserved. The candidate suffix is checked +against every name the file claims, so the third column becomes `A_3`. + +### distinct_headers_are_not_renamed_and_warn_nothing + +The negative arm: an ordinary file must not acquire suffixes or warnings. +Without it, renaming every column would satisfy the two tests above. + +### a_row_wider_than_the_header_is_reported + +The half of improvise-k8i that was not closed with it. `.flexible(true)` also +permits a row *longer* than the header, and the insert loop guards with +`headers.get(i)`, which returns `None` — so every field past the header width was +dropped with no error, no warning and no record of it. The short-row half got +padding and a per-column warning in f0b9227; the long-row half got nothing, +because the ticket's title said "row widths mismatch" and only one direction was +reproduced. + +The values are **still dropped** — preserving them means inventing column names, +which changes the imported model's shape and is a wizard decision rather than a +parser one. What changed is that it is no longer silent: the warning counts the +affected rows, names the header width and gives the widest row seen, which is +enough for a user to tell a stray delimiter from a genuinely different file. + +**Fault injected:** the `record.len() > headers.len()` counter disabled. Both +this test and `long_csv_row_is_not_silently_dropped` fail. + --- ## `improvise-io/src/import/wizard.rs` @@ -648,6 +943,56 @@ the CSV parser already turns a blank field into `Value::Null`, so the two inputs must agree. An item with an empty name is unnameable in the grid, so it is treated as missing rather than created. +### build_model_sums_records_that_share_every_coordinate + +improvise-2cw9, rated P1. Each record called `set_cell` directly, so two source +rows with identical category values — `East,10` then `East,20` — were **one** +cell and the second overwrote the first. A sum over the import read 20 where the +file said 30, with no error and no warning, and nothing downstream could tell +that a row had existed. + +**The policy is a decision, and the ticket deliberately left it open**: preserve, +aggregate, or reject. The model cannot preserve — a cell is named by its +coordinate tuple and there is no second address for a second record, so the only +lossless options are aggregating or inventing a row-identity category, and the +latter changes the shape of every imported document. Summing is what a pivot +table does to repeated records and is `measure_agg`'s own default, so it is the +answer here; the warning is what keeps it from being silent, because it is *not* +right for a measure the user will later set to `Avg` (that value is now +pre-summed). If that case matters, the follow-up is a wizard step that asks, not +a change to this default. + +**Fault injected:** the accumulate arm changed to `total = val; contributors = 1` +— last-row-wins again. This test fails on `Some(30.0)`, as does +`repeated_import_coordinates_not_silently_overwritten`. + +The `West` cell is asserted alongside, because a repair that summed *every* cell +into one, or that dropped the singleton path, would otherwise pass. The warning +is asserted **not** to name `West`, so the collision report cannot be a list of +every cell in the document. + +### build_model_writes_cells_in_first_seen_order + +Not a reported bug — the hazard the 2cw9 fix introduced. Accumulating means a +map keyed by `CellKey`, and gotcha 18 is exactly this: `Model::set_cell` writes +into an `IndexMap` whose order persistence depends on, so writing the cells in a +`HashMap`'s iteration order would have given the same CSV a different `.improv` +file per process. That is improvise-7yx's bug, in a third place, introduced by +the fix for a different one. The `HashMap` here only deduplicates; the order is +carried by a `Vec` beside it. + +**Fault injected:** `cells.sort_by_key(|c| c.key.to_string())` before the write +loop — a *deterministic* reordering, which is the weaker fault and still caught. +Five iterations, because each `HashMap` gets its own `RandomState` and one run +of a hash order can happen to be right. + +### a_long_collision_list_is_truncated_with_a_count + +The warning is rendered into a one-line status bar. A file with ten thousand +duplicate coordinates would otherwise compose a warning naming every one. Three +cells are named and the rest counted; the empty case returns `None` rather than +a warning that says nothing happened. + ### build_model_warns_about_records_skipped_for_missing_category improvise-5j9: skipping a record for a missing category value used to be silent, diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index b6dd980a..cf381065 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -342,6 +342,16 @@ pub struct App { pub storage: Box, } +/// The document a transaction was opened against: the workbook, its dirty marker +/// and the two counted tiers. What is NOT here: `context/code/command.md`. +#[derive(Debug, Clone)] +pub struct DocumentSnapshot { + workbook: Workbook, + dirty: bool, + document_generation: u64, + layout_version: u64, +} + impl App { /// Build an app backed by the real filesystem. pub fn new(workbook: Workbook, file_path: Option) -> Self { @@ -432,6 +442,30 @@ impl App { self.batch_aborted } + /// The document and its basis tiers, kept so a failed frame can put them + /// back (ADR 001 clause 4). See `context/code/command.md`. + pub fn snapshot_document(&self) -> DocumentSnapshot { + DocumentSnapshot { + workbook: self.model_state.workbook.clone(), + dirty: self.model_state.dirty, + document_generation: self.document_generation, + layout_version: self.layout_version, + } + } + + /// Put the document back as `snapshot` described it. **The document and its + /// tiers only** — `view_state` is deliberately untouched (see the doc). + pub fn restore_document(&mut self, snapshot: DocumentSnapshot) { + self.model_state.workbook = snapshot.workbook; + self.model_state.dirty = snapshot.dirty; + self.document_generation = snapshot.document_generation; + self.layout_version = snapshot.layout_version; + self.last_reshaped_views.clear(); + self.last_reframed_views.clear(); + self.last_reframed_elsewhere.clear(); + self.rebuild_layout(); + } + /// How many times this app's document has been replaced. A session that /// was seeded at an earlier generation is holding another document. pub fn document_generation(&self) -> u64 { diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 51ff2e03..c46d4d17 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -4,7 +4,7 @@ use crate::ui::app::App; use crate::ui::effect; use crate::ui::project::{Applied, Broadcast, Dirty, SessionId, Sessions}; -use crate::ui::reduce::{Checked, CheckedBatch}; +use crate::ui::reduce::{self, Checked, CheckedBatch}; use improvise_protocol::{Command, Rejection}; use improvise_protocol::{ClientFrame, ServerFrame}; @@ -139,13 +139,44 @@ impl Host { }]; } }; + // Clause 4: the document AND every mirror go back if any of it fails, + // so nothing a failed frame did is observable — `context/code/command.md`. + let undo = self.app.snapshot_document(); + let mirrors = self.sessions.clone(); + let basis = batch.basis().cloned(); let mut out = Vec::new(); for cmd in batch { + let broke = basis + .as_ref() + .and_then(|basis| reduce::broken_chain(&self.app, basis, cmd.command())); + if let Some(reason) = broke { + return self.undo_frame(id, undo, mirrors, reason); + } out.extend(self.command(id, cmd)); + if self.app.last_batch_aborted() { + return self.undo_frame(id, undo, mirrors, Rejection::MutationRefused); + } } out } + /// Put the document and every mirror back, and tell the asker alone: the + /// frame did nothing, so nobody else is owed anything (ADR 001 clause 4). + fn undo_frame( + &mut self, + id: SessionId, + undo: crate::ui::app::DocumentSnapshot, + mirrors: Sessions, + reason: Rejection, + ) -> Vec { + self.app.restore_document(undo); + self.sessions = mirrors; + vec![Addressed { + to: id, + msg: ServerFrame::Rejected(reason), + }] + } + /// The one door (gotcha 36); the match that follows is the mechanism (gotcha 39): /// asking for the `Dirty` means naming the case where a `Snapshot` is owed. fn command(&mut self, id: SessionId, cmd: Checked<'_>) -> Vec { @@ -781,10 +812,10 @@ mod tests { ); } - /// **Characterization of the open hole (clause 4), not an endorsement.** Invert - /// all three assertions when clause 4 lands — see the evidence. + /// ADR 001 clause 4. The inversion of the hole this characterized until + /// improvise-yqpd — see `context/test-evidence/command.md`. #[test] - fn a_frame_whose_later_mutation_fails_is_not_yet_undone() { + fn a_frame_whose_later_mutation_fails_is_undone_whole() { use improvise_protocol::ModelCommand; let mut host = host(); @@ -798,6 +829,7 @@ mod tests { ); // The frame claims North at its current counter, so it passes the check and // fails at *apply* time — the only way to reach the hole. + let before_version = host.app().record_version_of(addressed("North").record); let basis = frame_basis(&host, &["North"]); let answer = host.handle( asker, @@ -820,14 +852,14 @@ mod tests { ); assert!( - !host - .app() + host.app() .model_state .workbook .model .category("Region") .is_some_and(|c| c.ordered_item_names().contains(&"North")), - "TODAY: the first command lands. Under clause 4 it must be undone." + "the first command must be UNDONE: the frame failed, so the base \ + it was composed against is what the document must read as" ); assert_eq!( host.app() @@ -835,19 +867,130 @@ mod tests { .workbook .model .get_cell(&cell("North")), - None, - "TODAY: the second mutation does NOT land — its surrogate no \ - longer resolves. **This is the assertion that makes the test \ - about the hole**: without it the test passes against a frame \ - that succeeded completely, which is how it was found to be \ - vacuous." + Some(&CellValue::Number(1.0)), + "and the cell reads its PRE-FRAME value — not 5.0 (the frame's \ + own write landing) and not None (the removal surviving). This is \ + the assertion that separates a rollback from an abort." ); + assert_eq!( + host.app().record_version_of(addressed("North").record), + before_version, + "the record tier goes back too, or the client's next batch is \ + refused for a write that never happened" + ); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::ChainBroken(_)))), + "and the frame is refused OUT LOUD, naming the chain it broke: {answer:?}" + ); + } + + /// The rollback reaches the MIRRORS, not only the document — the cost the + /// ticket called its least obvious. See `context/test-evidence/command.md`. + #[test] + fn a_failed_frame_puts_the_askers_mirror_back_on_the_view_it_was_on() { + use improvise_protocol::ModelCommand; + + let mut host = host(); + let (asker, _) = host.connect(10, 10); + host.app_mut() + .model_state + .workbook + .views + .insert("Second".to_string(), crate::view::View::new("Second")); + host.model( + asker, + ModelCommand::SetCell { + cell: addressed("North"), + value: CellValue::Number(1.0), + }, + ); + let was = host + .sessions + .get_mut(asker) + .map(|s| s.view().active_view.clone()); + assert_eq!( + was, + Some("Default".to_string()), + "the fixture must start somewhere the switch can move it FROM, or \ + the assertion below holds for a mirror nothing ever wrote" + ); + // A switch moves ONLY the asker's mirror and is relayed to nobody + // (gotcha 36), so nothing in the stream can ever correct a stale one. + let basis = frame_basis(&host, &["North"]); + host.handle( + asker, + ClientFrame::Commands(vec![ + Command::SwitchView("Second".into()), + Command::Model { + basis: basis.clone(), + cmd: ModelCommand::RemoveItem { + category: "Region".into(), + item: "North".into(), + }, + }, + Command::Model { + basis, + cmd: ModelCommand::SetCell { + cell: addressed("North"), + value: CellValue::Number(5.0), + }, + }, + ]), + ); + + assert_eq!( + host.sessions + .get_mut(asker) + .map(|s| s.view().active_view.clone()), + was, + "the switch is part of the frame, so a refused frame leaves the \ + mirror where it started — and an accepted switch is relayed to \ + nobody, so a surviving one diverges from the client permanently" + ); + } + + /// Clause 4 must not fire on a frame that merely names a record it is + /// creating. See `context/test-evidence/command.md`. + #[test] + fn a_frame_that_creates_the_record_it_writes_is_not_a_broken_chain() { + use improvise_protocol::ModelCommand; + + let mut host = host(); + let (asker, _) = host.connect(10, 10); + let add = crate::ui::reduce::caught_up( + host.app(), + ModelCommand::AddItem { + category: "Region".into(), + item: "West".into(), + }, + ); + let write = crate::ui::reduce::caught_up( + host.app(), + ModelCommand::SetCell { + cell: addressed("West"), + value: CellValue::Number(7.0), + }, + ); + + let answer = host.handle(asker, ClientFrame::Commands(vec![add, write])); + assert!( !answer .iter() .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), - "TODAY: the frame is reported accepted although half of it did \ - nothing. Under clause 4 it must be refused: {answer:?}" + "the exemption is a property — a record neither resolving nor \ + claimed is one the frame creates: {answer:?}" + ); + assert_eq!( + host.app() + .model_state + .workbook + .model + .get_cell(&cell("West")), + Some(&CellValue::Number(7.0)), + "and clause 4 must not have rolled it back" ); } diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 3acd0663..a63c125c 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -674,7 +674,7 @@ pub fn grid_updates(mirror: &GridCache, desired: &GridCache) -> Vec /// One connected client, as the server sees it: the mirror of its [`ViewState`], /// kept by folding the same commands it folds, and the grid it is believed to hold. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ClientSession { view: ViewState, extent: (usize, usize), @@ -712,8 +712,12 @@ impl ClientSession { /// same function the client runs. Private: [`Sessions::apply`] is the door. fn fold(&mut self, cmd: &Command) { reduce_view(&mut self.view, cmd); - // The mirror settles exactly as the client does, against the grid this - // session was last projected (improvise-e5pf). + self.settle_mode(); + } + + /// Settle against the grid this mirror **holds**, after a fold and after + /// every projection that moves it (improvise-e5pf/el9h). + fn settle_mode(&mut self) { self.view.mode = improvise_protocol::settled_mode(&self.view.mode, self.grid.records); } @@ -769,6 +773,9 @@ impl ClientSession { self.grid = desired; self.projected = Some(viewport); self.projected_view = Some(self.view.active_view.clone()); + // The client re-settles on every fold, this projection included, so + // a mirror that settled only in `fold` diverges (improvise-el9h). + self.settle_mode(); } out.extend(self.basis_update(app).map(Projection::Basis)); out @@ -826,7 +833,7 @@ impl ClientSession { pub struct SessionId(pub u64); /// Every client attached to one authoritative `App`. -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub struct Sessions { next_id: u64, sessions: BTreeMap, @@ -1436,6 +1443,75 @@ mod tests { app } + /// A pivot-shaped default view plus a records-shaped second one, so a + /// switch between them crosses the boundary `settled_mode` watches. + fn app_with_a_records_shaped_second_view() -> App { + let mut app = app_with_data(); + // Cloned, not `View::new`: a fresh view has no axis entry for the + // virtual categories, so `is_records_mode` never fires on one. + let mut records = app.model_state.workbook.active_view().clone(); + records.name = "Records".to_string(); + records.set_axis("_Index", Axis::Row); + records.set_axis("_Dim", Axis::Column); + records.set_axis("Region", Axis::None); + records.set_axis("_Measure", Axis::None); + app.model_state + .workbook + .views + .insert("Records".to_string(), records); + app.rebuild_layout(); + assert!( + grid_in(&app, "Records", full_viewport()).records, + "the second view must really reach records mode, or the test \ + asserts a settle that had nothing to settle" + ); + app + } + + /// improvise-el9h: the mirror must settle against the grid it ends up + /// holding. Evidence: `context/test-evidence/command.md`. + #[test] + fn the_mirror_settles_its_mode_against_every_grid_it_is_later_projected() { + use improvise_protocol::AppMode; + + let mut app = app_with_a_records_shaped_second_view(); + let mut sessions = Sessions::default(); + let (id, snapshot) = sessions.connect(&app, 100, 100); + let mut client = improvise_protocol::ClientState::default(); + client.bootstrap(snapshot); + assert_eq!( + client.view.mode, + AppMode::Normal, + "the fixture must START on the pivot view, or the settle has \ + nowhere to move the mode from" + ); + + for cmd in [ + Command::SwitchView("Records".to_string()), + Command::ChangeMode(AppMode::Normal), + ] { + sessions.apply(&mut app, id, &cmd); + } + for (_, projections) in sessions.project_all(&app, Dirty::EVERYTHING) { + for projection in projections { + client.fold(&Command::Project(projection)); + } + } + + assert_eq!( + client.view.mode, + AppMode::RecordsNormal, + "the CLIENT re-settles on every fold, the projected grid included, \ + so it is the yardstick and must have moved" + ); + assert_eq!( + sessions.get_mut(id).map(|s| s.view().mode.clone()).as_ref(), + Some(&client.view.mode), + "and the mirror is the same function run on the same stream — it \ + settled against the grid it was LAST projected and never again" + ); + } + /// The yardstick `server_truth` used to be, spelled out instead of derived: /// see `context/test-evidence/command.md`. #[test] diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 5b76fe06..a93e2993 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -16,9 +16,20 @@ pub struct Checked<'a>(&'a Command); /// A whole frame, answered before any of it is applied (improvise-955n/08wb, /// ADR 001). Its four clauses are in `context/code/command.md`. -pub struct CheckedBatch<'a>(Vec>); +pub struct CheckedBatch<'a> { + checked: Vec>, + /// The one basis the frame stated, if any of its commands stated one. + /// Clause 4 asks it again at apply time — see `context/code/command.md`. + basis: Option, +} impl<'a> CheckedBatch<'a> { + /// What this frame was composed against. `None` for a frame that stated + /// none, which claims no record and so can break no chain. + pub fn basis(&self) -> Option<&Basis> { + self.basis.as_ref() + } + /// The document tiers are answered once, on the first command that /// carries a basis; the record tier is asked of every mutation. pub fn against(app: &App, cmds: &'a [Command]) -> Result { @@ -38,16 +49,34 @@ impl<'a> CheckedBatch<'a> { }; checked.push(one); } - Ok(Self(checked)) + Ok(Self { + checked, + basis: frame_basis.cloned(), + }) } } +/// The **structural** half of the record tier, asked again just before each +/// mutation runs. Clause 4's loud failure — `context/code/command.md`. +pub fn broken_chain(app: &App, basis: &Basis, cmd: &Command) -> Option { + let Command::Model { cmd: model, .. } = cmd else { + return None; + }; + records_named(model) + .into_iter() + // The SAME predicate the pre-frame check exempts by, not a second copy + // of it: two answers to "is the frame creating this" would drift. + .filter(|hash| !will_be_created_by_the_frame(app, basis, *hash)) + .find(|hash| app.resolve_record(*hash).is_none()) + .map(Rejection::ChainBroken) +} + impl<'a> IntoIterator for CheckedBatch<'a> { type Item = Checked<'a>; type IntoIter = std::vec::IntoIter>; fn into_iter(self) -> Self::IntoIter { - self.0.into_iter() + self.checked.into_iter() } } diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index 27f25a95..c6ca72c0 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -62,7 +62,7 @@ pub fn grid_body( true => value.as_ref().map(|v| v.to_string()).unwrap_or_default(), false => crate::format::format_value(value.as_ref(), fmt_comma, fmt_decimals), }; - widen(&mut widths, ci, text.width() as u16); + widen(&mut widths, ci, text.width()); if let Some(sum) = totals.get_mut(ci) { *sum += value.as_ref().and_then(|v| v.as_f64()).unwrap_or(0.0); } @@ -75,7 +75,7 @@ pub fn grid_body( } } for (ci, total) in totals.iter().enumerate() { - let width = format_f64(*total, fmt_comma, fmt_decimals).width() as u16; + let width = format_f64(*total, fmt_comma, fmt_decimals).width(); widen(&mut widths, ci, width); } @@ -114,14 +114,18 @@ fn header_level_seed(layout: &GridLayout, n: usize) -> Vec { AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, }); for (ci, levels) in items.enumerate().take(n) { - widths[ci] = levels.iter().map(|s| s.width() as u16).max().unwrap_or(0); + for level in levels { + widen(&mut widths, ci, level.width()); + } } widths } -fn widen(widths: &mut [u16], col: usize, width: u16) { +/// Every measured width enters through here and is capped **while still +/// `usize`**, so the `+ 1` gap below cannot overflow (improvise-yae4). +fn widen(widths: &mut [u16], col: usize, width: usize) { if let Some(slot) = widths.get_mut(col) { - *slot = (*slot).max(width); + *slot = (*slot).max(width.min(MAX_COL_WIDTH as usize) as u16); } } diff --git a/crates/improvise-core/src/format.rs b/crates/improvise-core/src/format.rs index cbc1909a..cad9232a 100644 --- a/crates/improvise-core/src/format.rs +++ b/crates/improvise-core/src/format.rs @@ -25,10 +25,16 @@ pub fn parse_number_format(fmt: &str) -> (bool, u8) { (comma, precision.parse::().unwrap_or(0)) } -/// Round half away from zero (the "normal" rounding people expect). +/// Round half away from zero (the "normal" rounding people expect). Scaling +/// can overflow a finite `n`, and display never invents an infinity (pikk). fn round_half_away(n: f64, decimals: u8) -> f64 { let factor = 10_f64.powi(decimals as i32); - (n * factor + n.signum() * 0.5).trunc() / factor + let scaled = n * factor; + match scaled.is_finite() { + true => (scaled + n.signum() * 0.5).trunc() / factor, + // Beyond the scale, n has no fractional part left to round. + false => n, + } } /// Format an f64 with optional comma grouping and decimal places. @@ -272,4 +278,15 @@ mod tests { fn format_value_none() { assert_eq!(format_value(None, true, 2), ""); } + + /// improvise-pikk — see context/test-evidence/core.md. + #[test] + fn a_finite_number_never_displays_as_infinity() { + // 1e307 * 100 overflows f64; 1e306 * 100 does not. + assert_eq!(format_f64(1e307, false, 2), format!("{:.2}", 1e307)); + assert_ne!(format_f64(1e306, false, 2), "inf"); + // The guard did not swallow the rounding path: half-away, not + // Rust's half-to-even, which would print "2". + assert_eq!(format_f64(2.5, false, 0), "3"); + } } diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 723b07a5..49d767ae 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -63,24 +63,36 @@ fn eval_agg( eval_at: &dyn Fn(&Expr, &CellKey) -> Result, ) -> Result { let mut partial = context.without(target_category); - if let Expr::Ref(item_name) = inner - && let Some(cat) = model.find_item_category(item_name) - { - partial = partial.with(cat, item_name.as_str()); - } + // The fast path is the fold specialised to a ref that names a STORED + // cell; where it does not, the premise fails — §13, s08t, s2w3. + let select_raw = match inner { + Expr::Ref(item_name) => { + let cat = model + .find_item_category(item_name) + .ok_or_else(|| format!("ref:{item_name}"))?; + let selected = partial.clone().with(cat, item_name.as_str()); + // `formula_for` is the one home of "is this cell derived". + let derived = model.formula_for(&selected).is_some(); + if !derived { + partial = selected; + } + !derived + } + _ => false, + }; if let Some(f) = agg_filter { partial = partial.with(&f.category, &f.item); } - let values: Vec = match inner { + let values: Vec = match select_raw { // A bare ref asks for the values that exist; anything else asks the // expression of each member record and folds the answers — §13. - Expr::Ref(_) => model + true => model .data .matching_values(&partial.0) .into_iter() .filter_map(|v| v.as_f64()) .collect(), - _ => member_records(model, &partial) + false => member_records(model, &partial) .into_iter() .map(|record| { let at = record @@ -188,7 +200,12 @@ fn eval_bool( } } } - _ => Err("type".into()), + Expr::Number(_) + | Expr::Ref(_) + | Expr::UnaryMinus(_) + | Expr::Agg(..) + | Expr::Is(_) + | Expr::If(..) => Err("type".into()), } } @@ -271,6 +288,11 @@ impl Model { /// The caller registers it on every view — normally `Workbook`. pub fn add_category(&mut self, name: impl Into) -> Result { let name = name.into(); + // Idempotence first: re-adding an existing name creates nothing, so + // the limit has nothing to refuse (improvise-f86q). + if self.categories.contains_key(&name) { + return Ok(self.categories[&name].id); + } // Only regular pivot categories count against the limit (§9). let regular_count = self .categories @@ -280,9 +302,6 @@ impl Model { if regular_count >= MAX_CATEGORIES { return Err(anyhow!("Maximum of {MAX_CATEGORIES} categories reached")); } - if self.categories.contains_key(&name) { - return Ok(self.categories[&name].id); - } let id = self.next_category_id; self.next_category_id += 1; self.categories @@ -547,8 +566,9 @@ impl Model { } // Fixed-point iteration. + let mut moving: Vec = Vec::new(); for _pass in 0..Self::MAX_EVAL_DEPTH { - let mut changed = false; + moving.clear(); for (target_cat, target_name) in &formula_cats { for stem in &stems { let key = stem.clone().with(target_cat, target_name); @@ -557,17 +577,23 @@ impl Model { let new_val = self.evaluate_formula_cell(&key, none_cats); let old_val = self.formula_cache.get(&key); if old_val != new_val.as_ref() { - changed = true; + moving.push(key.clone()); if let Some(v) = new_val { self.formula_cache.insert(key, v); } } } } - if !changed { + if moving.is_empty() { break; } } + // A cell still moving after the bound never converged: publishing the + // last iteration is publishing a plausible wrong number (§3, 7iww). + for key in moving.drain(..) { + self.formula_cache + .insert(key, CellValue::Error("circular".into())); + } } /// One formula cell for the fixed-point pass: raw aggregation for a @@ -697,10 +723,17 @@ impl Model { BinOp::Gt => Ok(lv > rv), BinOp::Le => Ok(lv <= rv), BinOp::Ge => Ok(lv >= rv), - _ => Err("type".into()), + BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div | BinOp::Pow => { + Err("type".into()) + } } } - _ => Err("type".into()), + Expr::Number(_) + | Expr::Ref(_) + | Expr::UnaryMinus(_) + | Expr::Agg(..) + | Expr::Is(_) + | Expr::If(..) => Err("type".into()), } } @@ -791,11 +824,13 @@ mod agg_helper_tests { /// — see context/test-evidence/core.md. #[test] fn eval_agg_over_no_rows_is_zero_only_where_an_identity_exists() { - let model = Model::new("T"); - let missing = Expr::Ref("Nonexistent".to_string()); + let mut model = Model::new("T"); + model.category_mut("_Measure").unwrap().add_item("Revenue"); + // A name that RESOLVES and has no cells: an unknown one is s08t. + let empty = Expr::Ref("Revenue".to_string()); let ctx = CellKey::new(vec![]); let never = |_: &Expr, _: &CellKey| unreachable!("a bare Ref never folds per record"); - let run = |func| eval_agg(&model, &ctx, "_Measure", &func, &missing, None, &never); + let run = |func| eval_agg(&model, &ctx, "_Measure", &func, &empty, None, &never); assert_eq!(run(AggFunc::Sum), Ok(0.0)); assert_eq!(run(AggFunc::Count), Ok(0.0)); @@ -846,6 +881,155 @@ mod model_tests { ) } + /// One stored `Revenue = 10` under `Region=East`, the shape every bug + /// in this group was demonstrated on. + fn revenue_model() -> Model { + let mut m = Model::new("T"); + m.add_category("Region").unwrap(); + m.category_mut("Region").unwrap().add_item("East"); + m.category_mut("_Measure").unwrap().add_item("Revenue"); + m.set_cell( + coord(&[("Region", "East"), ("_Measure", "Revenue")]), + CellValue::Number(10.0), + ); + m + } + + /// improvise-f86q — see context/test-evidence/core.md. + #[test] + fn re_adding_an_existing_category_at_the_limit_creates_nothing_and_succeeds() { + let mut m = Model::new("T"); + for i in 0..super::MAX_CATEGORIES { + m.add_category(format!("C{i}")).unwrap(); + } + let before = m.categories.len(); + let id = m.add_category("C0").expect("C0 already exists"); + assert_eq!(id, m.categories["C0"].id); + assert_eq!(m.categories.len(), before, "nothing was created"); + // The limit still refuses a category that WOULD be created. + assert!(m.add_category("C12").is_err(), "the limit still holds"); + } + + /// improvise-s08t — see context/test-evidence/core.md. + #[test] + fn an_unknown_name_inside_an_aggregate_errors_for_every_function() { + for func in ["SUM", "AVG", "MIN", "MAX", "COUNT"] { + let mut m = revenue_model(); + m.add_formula( + crate::formula::parse_formula(&format!("Total = {func}(Typo)"), "_Measure") + .unwrap(), + ); + let key = coord(&[("Region", "East"), ("_Measure", "Total")]); + assert!( + matches!(m.evaluate(&key), Some(CellValue::Error(ref e)) if e == "ref:Typo"), + "{func}(Typo) direct: {:?}", + m.evaluate(&key) + ); + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + let cached = m.evaluate_aggregated(&key, Aggregation::OverHidden); + assert!( + matches!(cached, Some(CellValue::Error(ref e)) if e == "ref:Typo"), + "{func}(Typo) cached: {cached:?}" + ); + } + } + + /// improvise-s2w3 — see context/test-evidence/core.md. + #[test] + fn an_aggregate_over_a_derived_measure_sees_the_formula_in_both_evaluators() { + for func in ["SUM", "AVG", "MIN", "MAX"] { + let mut m = revenue_model(); + for raw in [ + "Double = Revenue * 2".to_string(), + format!("A = {func}(Double)"), + format!("B = {func}(Double + 0)"), + ] { + m.add_formula(crate::formula::parse_formula(&raw, "_Measure").unwrap()); + } + let stem = coord(&[("Region", "East")]); + let a = stem.clone().with("_Measure", "A"); + let b = stem.with("_Measure", "B"); + assert_eq!( + m.evaluate(&a), + Some(CellValue::Number(20.0)), + "{func}(Double) direct" + ); + assert_eq!(m.evaluate(&a), m.evaluate(&b), "{func} direct vs identity"); + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + assert_eq!( + m.evaluate_aggregated(&a, Aggregation::OverHidden), + m.evaluate_aggregated(&b, Aggregation::OverHidden), + "{func} cached vs identity" + ); + } + } + + /// improvise-s2w3: a formula target shadowing stored data reads as the + /// formula — see context/test-evidence/core.md. + #[test] + fn a_derived_measure_with_stored_cells_does_not_leak_the_raw_value() { + let mut m = revenue_model(); + m.category_mut("_Measure").unwrap().add_item("Double"); + m.set_cell( + coord(&[("Region", "East"), ("_Measure", "Double")]), + CellValue::Number(999.0), + ); + m.add_formula(crate::formula::parse_formula("Double = Revenue * 2", "_Measure").unwrap()); + m.add_formula(crate::formula::parse_formula("A = SUM(Double)", "_Measure").unwrap()); + let a = coord(&[("Region", "East"), ("_Measure", "A")]); + assert_eq!(m.evaluate(&a), Some(CellValue::Number(20.0))); + } + + /// improvise-7iww: the non-convergence marker does not swallow a chain + /// that converges — see context/test-evidence/core.md. + #[test] + fn a_chained_formula_still_converges_after_the_circular_marker() { + let mut m = revenue_model(); + for raw in [ + "Cost = Revenue / 2", + "Profit = Revenue - Cost", + "Margin = Profit / Revenue", + ] { + m.add_formula(crate::formula::parse_formula(raw, "_Measure").unwrap()); + } + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + let stem = coord(&[("Region", "East")]); + for (target, want) in [("Cost", 5.0), ("Profit", 5.0), ("Margin", 0.5)] { + assert_eq!( + m.evaluate_aggregated( + &stem.clone().with("_Measure", target), + Aggregation::OverHidden + ), + Some(CellValue::Number(want)), + "{target}" + ); + } + } + + /// improvise-vyrx: the two evaluators must agree arm for arm — see + /// context/test-evidence/core.md. + #[test] + fn both_evaluators_answer_every_comparison_the_same_way() { + for op in ["=", "!=", "<", ">", "<=", ">="] { + for rhs in ["10", "5"] { + let mut m = revenue_model(); + m.add_formula( + crate::formula::parse_formula( + &format!("Flag = IF(Revenue {op} {rhs}, 1, 0)"), + "_Measure", + ) + .unwrap(), + ); + let key = coord(&[("Region", "East"), ("_Measure", "Flag")]); + let direct = m.evaluate(&key); + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + let cached = m.evaluate_aggregated(&key, Aggregation::OverHidden); + assert_eq!(direct, cached, "Revenue {op} {rhs}"); + assert!(matches!(direct, Some(CellValue::Number(_))), "{op} {rhs}"); + } + } + } + /// improvise-2lh: what makes `of_name` sound as a substitute — see /// context/test-evidence/core.md. #[test] diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 4cdd314c..b5591f4a 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -126,27 +126,52 @@ fn data_index_to_visual(items: &[AxisEntry], data_idx: usize) -> Option { .nth(data_idx) } +/// The run a header opened has ended. A header still pending saw either no +/// item at all — it is collapsed, and survives — or only rejected ones. +fn close_pending_run( + kept: &mut Vec, + pending: Option, + had_item: bool, +) -> bool { + pending + .filter(|_| !had_item) + .map(|h| kept.push(h)) + .is_some() +} + /// Drop the data items `keep` rejects, keeping a header only if an item under -/// it survives. A predicate, not a flag vector — why: core.md §6. +/// it survives or the group is collapsed. A predicate — why: core.md §6. fn prune_axis_items(items: &mut Vec, keep: impl Fn(usize) -> bool) { - let mut kept = Vec::new(); + let mut kept: Vec = Vec::new(); let mut pending_header: Option = None; + let mut run_had_item = false; + let mut run_open = false; let mut data_idx = 0usize; for entry in items.drain(..) { match &entry { + // A header ends the previous run: another group's header is where + // `expand_category` stops emitting a `GroupEnd` (improvise-gd83). AxisEntry::GroupHeader { .. } => { + run_open |= close_pending_run(&mut kept, pending_header.take(), run_had_item); pending_header = Some(entry); + run_had_item = false; + } + // Kept whenever a run is open in `kept`, even where the header it + // closed was pruned: an earlier one is then re-opened (u6gc). + AxisEntry::GroupEnd => { + run_open |= close_pending_run(&mut kept, pending_header.take(), run_had_item); + run_had_item = false; + if run_open { + kept.push(entry); + run_open = false; + } } - // The run it closes may have been pruned away entirely, in which - // case the header is still pending and both are dropped together. - AxisEntry::GroupEnd => match pending_header.take() { - Some(_) => {} - None => kept.push(entry), - }, AxisEntry::DataItem(_) => { + run_had_item = true; if keep(data_idx) { if let Some(h) = pending_header.take() { kept.push(h); + run_open = true; } kept.push(entry); } @@ -154,6 +179,7 @@ fn prune_axis_items(items: &mut Vec, keep: impl Fn(usize) -> bool) { } } } + close_pending_run(&mut kept, pending_header.take(), run_had_item); *items = kept; } @@ -1316,4 +1342,96 @@ mod tests { let layout = GridLayout::new(&wb.model, wb.active_view()); assert_eq!(layout.col_group_for(0), None); } + + /// Fund exactly the `(Month, Type=Food)` cells named, so `prune_empty` + /// keeps the rows the fixture pays for. context/test-evidence/core.md. + fn grouped_month_book(months: &[(&str, Option<&str>)], funded: &[&str]) -> Workbook { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + for (name, group) in months { + match group { + Some(g) => wb.add_item_in_group("Month", *name, *g).unwrap(), + None => wb.add_item("Month", *name).unwrap(), + }; + } + wb.add_item("Type", "Food").unwrap(); + for m in funded { + wb.model.set_cell( + CellKey::new(vec![ + ("Month".into(), (*m).into()), + ("Type".into(), "Food".into()), + ]), + CellValue::Number(1.0), + ); + } + wb.active_view_mut().prune_empty = true; + wb + } + + fn has_header(items: &[AxisEntry], group: &str) -> bool { + items + .iter() + .any(|e| matches!(e, AxisEntry::GroupHeader { group_name, .. } if group_name == group)) + } + + /// improvise-lhi4: a COLLAPSED group contributes zero data items, so the + /// pruner must not read its empty run as "pruned away". + #[test] + fn a_collapsed_groups_header_survives_pruning() { + let mut wb = grouped_month_book(&[("Jan", Some("Q1")), ("Sep", None)], &["Jan", "Sep"]); + wb.active_view_mut().toggle_group_collapse("Month", "Q1"); + let layout = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + assert_eq!(layout.row_count(), 1, "only Sep is a data row"); + assert!( + has_header(&layout.row_items, "Q1"), + "Q1 is collapsed, not empty — its header is the only way back" + ); + assert_eq!( + layout.row_group_for(0), + None, + "Sep must not inherit the collapsed run" + ); + } + + /// improvise-lhi4's other half, and what separates the two cases: a header + /// whose items really did prune away still goes. + #[test] + fn a_header_whose_items_all_prune_away_is_dropped() { + let wb = grouped_month_book(&[("Jan", Some("Q1")), ("Mar", Some("Q2"))], &["Jan"]); + let layout = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + assert_eq!(layout.row_count(), 1, "only Jan is funded"); + assert!(has_header(&layout.row_items, "Q1"), "Jan survives under Q1"); + assert!( + !has_header(&layout.row_items, "Q2"), + "Q2's only item pruned away, so its header goes with it" + ); + } + + /// improvise-u6gc: pruning the intervening header re-opens the earlier + /// run, so the `GroupEnd` that closed it must survive. Regresses gd83. + #[test] + fn an_ungrouped_item_after_a_pruned_group_belongs_to_no_group() { + let wb = grouped_month_book( + &[ + ("Jan", Some("Q1")), + ("Feb", Some("Q1")), + ("Mar", Some("Q2")), + ("Sep", None), + ], + &["Jan", "Feb", "Sep"], + ); + let layout = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + assert_eq!(layout.row_count(), 3, "Mar pruned; Jan, Feb, Sep remain"); + assert_eq!( + layout.row_group_for(0), + Some(("Month".into(), "Q1".into())), + "precondition: Jan is in Q1, so the walk has somewhere wrong to land" + ); + assert_eq!( + layout.row_group_for(2), + None, + "Sep is in no group — Q2's GroupEnd closed Q1's re-opened run" + ); + } } diff --git a/crates/improvise-core/src/view/mod.rs b/crates/improvise-core/src/view/mod.rs index 5e061ce2..b6f45941 100644 --- a/crates/improvise-core/src/view/mod.rs +++ b/crates/improvise-core/src/view/mod.rs @@ -7,4 +7,4 @@ pub use layout::{ AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow, synthetic_record_info, }; -pub use types::{DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View}; +pub use types::{DEFAULT_VIEW, DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View}; diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index eace86e6..1efeeb3e 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -13,6 +13,10 @@ fn default_prune() -> bool { /// they put the never-persisted `_Index`/`_Dim` on axes (improvise-60z). pub const VIRTUAL_VIEW_NAMES: [&str; 2] = ["_Records", "_Drill"]; +/// The view `Workbook::new` seeds. `parse_md` must name the same one to tell a +/// seeded view from one the file asked for (improvise-m4rv). +pub const DEFAULT_VIEW: &str = "Default"; + /// Name of the view records mode switches to. pub const RECORDS_VIEW: &str = VIRTUAL_VIEW_NAMES[0]; /// Name of the view drill-down switches to. @@ -94,6 +98,22 @@ impl View { self.category_axes.shift_remove(cat_name); self.page_selections.remove(cat_name); self.hidden_items.remove(cat_name); + self.collapsed_groups.remove(cat_name); + } + + /// Forget an item this view names. A hidden or paged-to item that outlives + /// its removal comes back hidden when re-added (improvise-qx1z). + pub fn on_item_removed(&mut self, cat_name: &str, item_name: &str) { + if let Some(items) = self.hidden_items.get_mut(cat_name) { + items.remove(item_name); + } + if self + .page_selections + .get(cat_name) + .is_some_and(|s| s == item_name) + { + self.page_selections.remove(cat_name); + } } pub fn set_axis(&mut self, cat_name: &str, axis: Axis) { @@ -475,6 +495,54 @@ mod tests { assert!(!v.is_group_collapsed("Cat1", "G2")); } + /// Arm 2 of improvise-qx1z: the other three lines of `on_category_removed` + /// were right there. Evidence: `context/test-evidence/core.md`. + #[test] + fn on_category_removed_forgets_a_collapsed_group() { + let mut v = View::new("V"); + v.on_category_added("Month"); + v.toggle_group_collapse("Month", "Q1"); + v.on_category_removed("Month"); + v.on_category_added("Month"); + assert!( + !v.is_group_collapsed("Month", "Q1"), + "a re-added category came back with its group collapsed" + ); + } + + /// Arm 1 of improvise-qx1z, at the `View` level: removing an item must not + /// leave it hidden, or re-adding it brings back an invisible row. + #[test] + fn on_item_removed_forgets_a_hidden_item() { + let mut v = View::new("V"); + v.on_category_added("Region"); + v.hide_item("Region", "East"); + v.hide_item("Region", "West"); + v.on_item_removed("Region", "East"); + assert!(!v.is_hidden("Region", "East"), "East is still hidden"); + assert!( + v.is_hidden("Region", "West"), + "on_item_removed must forget one item, not the category" + ); + } + + /// The third question improvise-qx1z asked to check while there: a page + /// selection naming a removed item does not survive it either. + #[test] + fn on_item_removed_forgets_a_page_selection_naming_it() { + let mut v = View::new("V"); + v.on_category_added("Region"); + v.set_page_selection("Region", "East"); + v.on_item_removed("Region", "West"); + assert_eq!( + v.page_selection("Region"), + Some("East"), + "removing a different item must leave the selection alone" + ); + v.on_item_removed("Region", "East"); + assert_eq!(v.page_selection("Region"), None); + } + #[test] fn hide_and_show_item() { let mut v = View::new("Test"); diff --git a/crates/improvise-core/src/workbook.rs b/crates/improvise-core/src/workbook.rs index b47e120d..243708de 100644 --- a/crates/improvise-core/src/workbook.rs +++ b/crates/improvise-core/src/workbook.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; use crate::model::Model; use crate::model::category::{CategoryId, ItemId}; -use crate::view::{Axis, View}; +use crate::view::{Axis, DEFAULT_VIEW, View}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Workbook { @@ -22,11 +22,11 @@ impl Workbook { pub fn new(name: impl Into) -> Self { let model = Model::new(name); let mut views = IndexMap::new(); - views.insert("Default".to_string(), View::new("Default")); + views.insert(DEFAULT_VIEW.to_string(), View::new(DEFAULT_VIEW)); let mut wb = Self { model, views, - active_view: "Default".to_string(), + active_view: DEFAULT_VIEW.to_string(), }; for view in wb.views.values_mut() { for cat_name in wb.model.categories.keys() { @@ -91,6 +91,9 @@ impl Workbook { /// ignored, matching [`Model::remove_item`]. pub fn remove_item(&mut self, cat_name: &str, item_name: &str) { self.model.remove_item(cat_name, item_name); + for view in self.views.values_mut() { + view.on_item_removed(cat_name, item_name); + } } fn category_mut(&mut self, cat_name: &str) -> Result<&mut crate::model::category::Category> { @@ -260,6 +263,27 @@ mod tests { ); } + /// Arm 1 of improvise-qx1z through the facade: `Model::remove_item` touches + /// no view, so the cross-view sweep is `Workbook`'s job (gotcha 6). + #[test] + fn remove_item_unhides_it_in_every_view() { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.create_view("Other"); + for name in ["Default", "Other"] { + wb.views.get_mut(name).unwrap().hide_item("Region", "East"); + } + wb.remove_item("Region", "East"); + wb.add_item("Region", "East").unwrap(); + for name in ["Default", "Other"] { + assert!( + !wb.views[name].is_hidden("Region", "East"), + "{name} brought East back hidden" + ); + } + } + #[test] fn remove_item_on_unknown_category_is_a_no_op() { let mut wb = Workbook::new("Test"); diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index 29497863..f5aced6b 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -154,8 +154,9 @@ fn parse_agg_func(s: &str) -> AggFunc { "AVG" => AggFunc::Avg, "MIN" => AggFunc::Min, "MAX" => AggFunc::Max, - // COUNT is the only remaining alternative. - _ => AggFunc::Count, + "COUNT" => AggFunc::Count, + // A new agg_func alternative must be spelled here (improvise-492x). + other => unreachable!("{GRAMMAR_INVARIANT}: agg_func {other}"), } } @@ -185,8 +186,9 @@ fn parse_cmp_op(s: &str) -> BinOp { "<" => BinOp::Lt, ">" => BinOp::Gt, "<=" => BinOp::Le, - // ">=" is the only remaining alternative. - _ => BinOp::Ge, + ">=" => BinOp::Ge, + // A new cmp_op alternative must be spelled here (improvise-492x). + other => unreachable!("{GRAMMAR_INVARIANT}: cmp_op {other}"), } } @@ -266,9 +268,44 @@ where #[cfg(test)] mod tests { - use super::parse_formula; + use super::{parse_agg_func, parse_cmp_op, parse_formula}; use crate::{AggFunc, BinOp, Expr}; + /// improvise-492x — see context/test-evidence/formula-and-io.md. + #[test] + fn every_grammar_aggregate_and_comparison_is_spelled_out() { + assert!(matches!(parse_agg_func("SUM"), AggFunc::Sum)); + assert!(matches!(parse_agg_func("AVG"), AggFunc::Avg)); + assert!(matches!(parse_agg_func("MIN"), AggFunc::Min)); + assert!(matches!(parse_agg_func("MAX"), AggFunc::Max)); + assert!(matches!(parse_agg_func("COUNT"), AggFunc::Count)); + for (token, want) in [ + ("=", BinOp::Eq), + ("!=", BinOp::Ne), + ("<", BinOp::Lt), + (">", BinOp::Gt), + ("<=", BinOp::Le), + (">=", BinOp::Ge), + ] { + assert_eq!(parse_cmp_op(token), want, "cmp_op {token}"); + } + } + + /// improvise-492x: a new `agg_func` alternative refuses loudly rather + /// than becoming COUNT — see context/test-evidence/formula-and-io.md. + #[test] + #[should_panic(expected = "grammar invariant violated")] + fn an_aggregate_the_walker_does_not_know_is_not_count() { + let _ = parse_agg_func("MEDIAN"); + } + + /// improvise-492x: and a new `cmp_op` alternative is not `>=`. + #[test] + #[should_panic(expected = "grammar invariant violated")] + fn a_comparison_the_walker_does_not_know_is_not_ge() { + let _ = parse_cmp_op("<>"); + } + #[test] fn parse_simple_subtraction() { let f = parse_formula("Profit = Revenue - Cost", "Foo").unwrap(); diff --git a/crates/improvise-io/src/import/analyzer.rs b/crates/improvise-io/src/import/analyzer.rs index a066534a..e688bef0 100644 --- a/crates/improvise-io/src/import/analyzer.rs +++ b/crates/improvise-io/src/import/analyzer.rs @@ -64,10 +64,10 @@ pub fn detect_date_format(samples: &[&str]) -> Option { if samples.is_empty() { return None; } - // Try up to 10 samples for efficiency - let test_samples: Vec<&str> = samples.into_iter().take(10).collect(); + // Every sample, not a window: a disambiguating value after a 10-value window + // is what decides between %m/%d/%Y and %d/%m/%Y (improvise-jcfs). for fmt in DATE_FORMATS { - if test_samples + if samples .iter() .all(|s| NaiveDate::parse_from_str(s, fmt).is_ok()) { @@ -118,8 +118,11 @@ pub fn analyze_records(records: &[Value]) -> Vec { .map(|field| { let values: Vec<&Value> = records.iter().filter_map(|r| r.get(&field)).collect(); - let all_numeric = values.iter().all(|v| v.is_number()); - let all_string = values.iter().all(|v| v.is_string()); + // Infer from the values PRESENT: one Null used to demote a numeric + // column to a Label; all-null falls through to Label (improvise-l1f5). + let present: Vec<&Value> = values.iter().copied().filter(|v| !v.is_null()).collect(); + let all_numeric = !present.is_empty() && present.iter().all(|v| v.is_number()); + let all_string = !present.is_empty() && present.iter().all(|v| v.is_string()); if all_numeric { return FieldProposal { @@ -136,7 +139,7 @@ pub fn analyze_records(records: &[Value]) -> Vec { // First-seen order, not hash order (gotcha 18, improvise-7yx): // the set deduplicates and is never iterated. let mut seen: HashSet<&str> = HashSet::new(); - let distinct_vec: Vec = values + let distinct_vec: Vec = present .iter() .filter_map(|v| v.as_str()) .filter(|s| seen.insert(s)) @@ -344,15 +347,15 @@ mod tests { } } - /// The same bug reaching the detected format, because only the first ten - /// samples are inspected. Evidence: the test-evidence doc. + /// The same bug reaching the detected format. The expected answer moved + /// with improvise-jcfs — see the test-evidence doc. #[test] fn the_detected_date_format_does_not_depend_on_the_hash_seed() { let mut vals: Vec = (1..=12).map(|d| format!("01/{d:02}/2025")).collect(); vals.push("25/12/2025".to_string()); assert!( vals.len() > 10, - "precondition: more values than detect_date_format looks at" + "precondition: more values than the old ten-sample window looked at" ); let records: Vec = vals .iter() @@ -362,7 +365,62 @@ mod tests { for _ in 0..5 { let proposals = analyze_records(&records); let when = proposals.iter().find(|p| p.field == "When").unwrap(); - assert_eq!(when.date_format, Some("%m/%d/%Y".to_string())); + assert_eq!(when.date_format, Some("%d/%m/%Y".to_string())); } } + + /// improvise-jcfs: `detect_date_format` sampled the first ten values, so a + /// disambiguating date after them could not be seen. + #[test] + fn a_disambiguating_date_after_the_eleventh_value_still_decides() { + let mut samples: Vec = (1..=11).map(|d| format!("01/{d:02}/2025")).collect(); + samples.push("25/12/2025".to_string()); + for s in samples.iter().take(11) { + assert!( + NaiveDate::parse_from_str(s, "%m/%d/%Y").is_ok() + && NaiveDate::parse_from_str(s, "%d/%m/%Y").is_ok(), + "precondition: {s} is ambiguous, so only the twelfth value can decide" + ); + } + let refs: Vec<&str> = samples.iter().map(String::as_str).collect(); + assert_eq!(detect_date_format(&refs), Some("%d/%m/%Y".to_string())); + } + + /// improvise-l1f5: `all_numeric` counted `Value::Null`, so one blank field + /// demoted an otherwise numeric column to a Label. + #[test] + fn a_blank_value_does_not_demote_a_numeric_column() { + let records: Vec = vec![ + serde_json::json!({"Revenue": 10, "Region": "East"}), + serde_json::json!({"Revenue": null, "Region": "West"}), + ]; + let proposals = analyze_records(&records); + let revenue = proposals.iter().find(|p| p.field == "Revenue").unwrap(); + assert_eq!(revenue.kind, FieldKind::Measure); + } + + /// The same defect one branch over: a blank category value made the whole + /// column a Label, and its items vanished from the proposal. + #[test] + fn a_blank_value_does_not_demote_a_string_column() { + let records: Vec = vec![ + serde_json::json!({"Region": "East"}), + serde_json::json!({"Region": null}), + serde_json::json!({"Region": "West"}), + ]; + let proposals = analyze_records(&records); + let region = proposals.iter().find(|p| p.field == "Region").unwrap(); + assert_eq!(region.kind, FieldKind::Category); + assert_eq!(region.distinct_values, ["East", "West"]); + } + + /// The all-null column has no evidence either way. Stated, because both + /// `all_numeric` and `all_string` are vacuously true over nothing. + #[test] + fn an_all_null_column_is_a_label() { + let records: Vec = vec![serde_json::json!({"Note": null, "Region": "East"})]; + let proposals = analyze_records(&records); + let note = proposals.iter().find(|p| p.field == "Note").unwrap(); + assert_eq!(note.kind, FieldKind::Label); + } } diff --git a/crates/improvise-io/src/import/csv_parser.rs b/crates/improvise-io/src/import/csv_parser.rs index 3171320c..0075740b 100644 --- a/crates/improvise-io/src/import/csv_parser.rs +++ b/crates/improvise-io/src/import/csv_parser.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::path::Path; use anyhow::{Context, Result}; @@ -41,23 +42,33 @@ pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(Vec, Vec = Vec::new(); + let mut header_warnings: Vec = Vec::new(); if has_headers { - headers = reader + let raw: Vec = reader .headers() .with_context(|| "Failed to read CSV headers")? .iter() .map(|s| s.to_string()) .collect(); + (headers, header_warnings) = disambiguate_headers(raw); } // Per-column count of rows that were too short to provide a value. let mut missing_counts = vec![0usize; headers.len()]; + // Rows carrying more fields than the header names, and the widest seen. + let mut long_rows = 0usize; + let mut widest_row = 0usize; for result in reader.records() { let record = result.with_context(|| "Failed to read CSV record")?; let mut map = serde_json::Map::new(); + if has_headers && record.len() > headers.len() { + long_rows += 1; + widest_row = widest_row.max(record.len()); + } + for (i, field) in record.iter().enumerate() { let json_value: Value = parse_csv_field(field); if has_headers { @@ -81,18 +92,56 @@ pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(Vec, Vec 0) - .map(|(header, count)| { - format!("column '{header}' missing from {count} row(s); filled with empty values") - }) - .collect(); + let mut warnings = header_warnings; + if long_rows > 0 { + warnings.push(format!( + "{long_rows} row(s) carried more fields than the {} header column(s) \ + (widest row: {widest_row} fields); the extra values were dropped", + headers.len() + )); + } + warnings.extend( + headers + .iter() + .zip(&missing_counts) + .filter(|&(_, &count)| count > 0) + .map(|(header, count)| { + format!("column '{header}' missing from {count} row(s); filled with empty values") + }), + ); Ok((records, warnings)) } +/// A header name keys the record object, so two equal names were one column and +/// the later one won silently (improvise-5hfh). Rename and report the later ones. +fn disambiguate_headers(raw: Vec) -> (Vec, Vec) { + let mut claimed: HashSet = raw.iter().cloned().collect(); + let mut seen: HashSet = HashSet::new(); + let mut headers = Vec::with_capacity(raw.len()); + let mut warnings = Vec::new(); + + for (i, name) in raw.into_iter().enumerate() { + if seen.insert(name.clone()) { + headers.push(name); + continue; + } + let renamed = (2usize..) + .map(|n| format!("{name}_{n}")) + .find(|candidate| !claimed.contains(candidate)) + .expect("an unclaimed suffix always exists"); + warnings.push(format!( + "duplicate header '{name}' at column {} renamed to '{renamed}'", + i + 1 + )); + claimed.insert(renamed.clone()); + seen.insert(renamed.clone()); + headers.push(renamed); + } + + (headers, warnings) +} + /// Parse multiple CSV files and merge into a single JSON array. /// Each record gets a "File" field set to the filename stem (e.g., "sales" from "sales.csv"). pub fn merge_csvs(paths: &[impl AsRef]) -> Result> { @@ -125,10 +174,12 @@ fn parse_csv_field(field: &str) -> Value { return Value::Number(serde_json::Number::from(num)); } - if let Ok(num) = field.parse::() { - return Value::Number( - serde_json::Number::from_f64(num).unwrap_or(serde_json::Number::from(0)), - ); + // NaN and the infinities have no JSON number. A fabricated 0 is an invented + // measurement (improvise-jc95), so the token falls through and stays text. + if let Ok(num) = field.parse::() + && let Some(number) = serde_json::Number::from_f64(num) + { + return Value::Number(number); } // Otherwise treat as string @@ -440,6 +491,75 @@ mod tests { } } + /// improvise-jc95: `Number::from_f64` has no answer for these, and the old + /// `unwrap_or(0)` invented a measurement. Evidence: the test-evidence doc. + #[test] + fn a_nonfinite_numeric_token_stays_text_rather_than_becoming_zero() { + for token in ["NaN", "inf", "-inf", "1e999", "-1e999"] { + assert!( + token.parse::().is_ok_and(|n| !n.is_finite()), + "precondition: {token} parses as a non-finite f64" + ); + assert_eq!(parse_csv_field(token), Value::String(token.to_string())); + } + } + + /// The control that makes the test above an assertion about non-finiteness + /// rather than about the float branch being dead. + #[test] + fn an_ordinary_float_is_still_a_number() { + assert_eq!( + parse_csv_field("1000.50"), + Value::Number(serde_json::Number::from_f64(1000.50).unwrap()) + ); + assert_eq!(parse_csv_field("0"), Value::Number(0.into())); + } + + /// improvise-5hfh: two equal header names are one JSON key, so the later + /// column overwrote the earlier one. Evidence: the test-evidence doc. + #[test] + fn a_repeated_header_is_renamed_rather_than_overwriting_its_twin() { + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B,A\n1,2,3\n").unwrap(); + let map = records[0].as_object().unwrap(); + assert_eq!(map["A"], Value::Number(1.into())); + assert_eq!(map["B"], Value::Number(2.into())); + assert_eq!(map["A_2"], Value::Number(3.into())); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("'A'") && warnings[0].contains("'A_2'")); + assert!(warnings[0].contains("column 3"), "{}", warnings[0]); + } + + /// The rename must not collide with a name the file already uses, or the + /// repair reintroduces the very overwrite it exists to prevent. + #[test] + fn a_rename_skips_a_suffix_the_file_already_claims() { + let (records, _) = parse_csv_bytes_with_warnings(b"A,A_2,A\n1,2,3\n").unwrap(); + let map = records[0].as_object().unwrap(); + assert_eq!(map.len(), 3, "{map:?}"); + assert_eq!(map["A_2"], Value::Number(2.into())); + assert_eq!(map["A_3"], Value::Number(3.into())); + } + + #[test] + fn distinct_headers_are_not_renamed_and_warn_nothing() { + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B\n1,2\n").unwrap(); + assert!(warnings.is_empty()); + let keys: Vec<&String> = records[0].as_object().unwrap().keys().collect(); + assert_eq!(keys, ["A", "B"]); + } + + /// improvise-k8i's remaining half: a row wider than the header had its + /// extra fields dropped by `headers.get(i)` returning `None`, silently. + #[test] + fn a_row_wider_than_the_header_is_reported() { + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B\n1,2\n1,2,3\n").unwrap(); + assert_eq!(records.len(), 2); + assert_eq!(records[1].as_object().unwrap().len(), 2); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("1 row(s)"), "{}", warnings[0]); + assert!(warnings[0].contains("widest row: 3"), "{}", warnings[0]); + } + #[test] fn parse_checking_csv_format() { // Simulates the format of /Users/edwlan/Downloads/Checking1.csv diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index 3fbe3682..e942240a 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use anyhow::{Result, anyhow}; use serde_json::Value; @@ -185,6 +187,10 @@ impl ImportPipeline { // value; a record is charged to the first category it is missing. let mut skipped_per_category = vec![0usize; categories.len()]; let mut skipped_non_objects = 0usize; + // Two rows sharing every coordinate are one cell; last-row-wins lost the + // earlier one (improvise-2cw9). The map only deduplicates — gotcha 18. + let mut cells: Vec = vec![]; + let mut cell_index: HashMap = HashMap::new(); for record in &self.records { if let Value::Object(map) = record { @@ -232,8 +238,21 @@ impl ImportPipeline { if let Some(val) = map.get(&measure.field).and_then(|v| v.as_f64()) { let mut cell_coords = coords.clone(); cell_coords.push(("_Measure".to_string(), measure.field.clone())); - wb.model - .set_cell(CellKey::new(cell_coords), CellValue::Number(val)); + let key = CellKey::new(cell_coords); + match cell_index.get(&key) { + Some(&at) => { + cells[at].total += val; + cells[at].contributors += 1; + } + None => { + cell_index.insert(key.clone(), cells.len()); + cells.push(PendingCell { + key, + total: val, + contributors: 1, + }); + } + } } } } else { @@ -241,6 +260,15 @@ impl ImportPipeline { } } + let collisions: Vec<(String, usize)> = cells + .iter() + .filter(|cell| cell.contributors > 1) + .map(|cell| (cell.key.to_string(), cell.contributors)) + .collect(); + for cell in cells { + wb.model.set_cell(cell.key, CellValue::Number(cell.total)); + } + // Parse and add formulas // Formulas target the "_Measure" category by default. let formula_cat: String = "_Measure".to_string(); @@ -250,17 +278,46 @@ impl ImportPipeline { } } - let warnings = skip_warnings( + let mut warnings = skip_warnings( self.records.len(), &categories, &skipped_per_category, skipped_non_objects, ); + warnings.extend(collision_warning(&collisions)); Ok((wb, warnings)) } } +/// One imported cell mid-accumulation: the running total and how many source +/// records reached it, so a collision can be reported rather than swallowed. +struct PendingCell { + key: CellKey, + total: f64, + contributors: usize, +} + +/// Repeated coordinate tuples are summed, never dropped, and the user is told +/// which cells that happened to (improvise-2cw9). Empty when none repeated. +fn collision_warning(collisions: &[(String, usize)]) -> Option { + let (first, rest) = collisions.split_at(collisions.len().min(3)); + let mut details: Vec = first + .iter() + .map(|(label, n)| format!("{label} ({n} records)")) + .collect(); + if !rest.is_empty() { + details.push(format!("and {} more", rest.len())); + } + (!collisions.is_empty()).then(|| { + format!( + "Summed {} cell(s) built from duplicate coordinates: {}", + collisions.len(), + details.join("; ") + ) + }) +} + /// The item name a JSON scalar contributes, or `None` when it carries no value. /// `Value::Null` and a blank string are absences, not names (improvise-ych). fn scalar_name(v: &Value) -> Option { @@ -696,9 +753,9 @@ impl ImportWizard { #[cfg(test)] mod tests { - use super::ImportPipeline; + use super::{ImportPipeline, collision_warning}; use crate::import::analyzer::FieldKind; - use serde_json::json; + use serde_json::{Value, json}; #[test] fn flat_array_auto_selected() { @@ -1670,4 +1727,78 @@ mod tests { Some(100.0) ); } + + /// improvise-2cw9: two rows sharing every coordinate are one cell, and the + /// later one overwrote the earlier. Evidence: the test-evidence doc. + #[test] + fn build_model_sums_records_that_share_every_coordinate() { + use crate::model::cell::CellKey; + let raw = json!([ + {"region": "East", "revenue": 10.0}, + {"region": "East", "revenue": 20.0}, + {"region": "West", "revenue": 5.0}, + ]); + let (wb, warnings) = ImportPipeline::new(raw) + .build_model_with_warnings() + .unwrap(); + + let key = |r: &str| { + CellKey::new(vec![ + ("region".into(), r.into()), + ("_Measure".into(), "revenue".into()), + ]) + }; + assert_eq!( + wb.model.get_cell(&key("East")).and_then(|v| v.as_f64()), + Some(30.0) + ); + assert_eq!( + wb.model.get_cell(&key("West")).and_then(|v| v.as_f64()), + Some(5.0), + "a coordinate reached once must not be touched by the repair" + ); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!(warnings[0].contains("region=East"), "{}", warnings[0]); + assert!(warnings[0].contains("(2 records)"), "{}", warnings[0]); + assert!( + !warnings[0].contains("West"), + "only the colliding cell is named: {}", + warnings[0] + ); + } + + /// The cells are written in first-seen order, not in the accumulator's hash + /// order (gotcha 18) — the same defect improvise-7yx fixed one file over. + #[test] + fn build_model_writes_cells_in_first_seen_order() { + let regions = ["hotel", "alpha", "golf", "bravo", "foxtrot", "charlie"]; + let raw = Value::Array( + regions + .iter() + .map(|r| json!({"region": r, "revenue": 1.0})) + .collect(), + ); + for _ in 0..5 { + let wb = ImportPipeline::new(raw.clone()).build_model().unwrap(); + let order: Vec = wb + .model + .data + .iter_cells() + .filter_map(|(k, _)| k.get("region").map(str::to_string)) + .collect(); + assert_eq!(order, regions); + } + } + + /// `collision_warning` names at most three cells and counts the rest, so a + /// wide collision cannot render an unreadable status line. + #[test] + fn a_long_collision_list_is_truncated_with_a_count() { + let collisions: Vec<(String, usize)> = (0..5).map(|i| (format!("c={i}"), 2)).collect(); + let warning = collision_warning(&collisions).unwrap(); + assert!(warning.contains("Summed 5 cell(s)"), "{warning}"); + assert!(warning.contains("and 2 more"), "{warning}"); + assert!(!warning.contains("c=3"), "{warning}"); + assert_eq!(collision_warning(&[]), None); + } } diff --git a/crates/improvise-io/src/persistence/format.rs b/crates/improvise-io/src/persistence/format.rs index 0ccd3dd3..1997009c 100644 --- a/crates/improvise-io/src/persistence/format.rs +++ b/crates/improvise-io/src/persistence/format.rs @@ -7,6 +7,7 @@ use crate::workbook::Workbook; // The one home of the pipe-quoting rule (improvise-i2m5); a name written here // must also survive `parse_formula`. See `context/code/formula-and-io.md`. +use crate::formula::AggFunc; use crate::formula::quote::{pipe_quote, quote_name}; /// A name as a section header spells it: the whole line, so no pipes where a @@ -22,6 +23,18 @@ fn quote_header(name: &str) -> String { } } +/// The on-disk keyword for an aggregation, exhaustively — a new `AggFunc` +/// variant fails the build here rather than being written as something else. +fn agg_keyword(agg: &AggFunc) -> &'static str { + match agg { + AggFunc::Sum => "sum", + AggFunc::Avg => "avg", + AggFunc::Min => "min", + AggFunc::Max => "max", + AggFunc::Count => "count", + } +} + fn format_number(n: f64) -> String { if n.is_infinite() { return if n.is_sign_positive() { @@ -108,6 +121,9 @@ pub fn format_md(workbook: &Workbook) -> String { if !view.number_format.is_empty() { w!(out, "format: {}", view.number_format); } + if view.prune_empty { + w!(out, "prune: true"); + } for (prefix, map) in [ ("hidden", &view.hidden_items), ("collapsed", &view.collapsed_groups), @@ -137,6 +153,19 @@ pub fn format_md(workbook: &Workbook) -> String { } } + // ── Aggregation ────────────────────────────────────────────────── + + // Sorted: `measure_agg` is a `HashMap` and hash order is not an order + // (gotcha 18) — two saves of one document must agree. + let mut aggs: Vec<_> = model.measure_agg.iter().collect(); + aggs.sort_by_key(|(name, _)| name.as_str()); + if !aggs.is_empty() { + w!(out, "\n## Aggregation"); + for (name, agg) in aggs { + w!(out, "- {}: {}", quote_name(name), agg_keyword(agg)); + } + } + // ── Categories (items comma-separated on one line) ─────────────── // Collect formula targets so we can exclude them from _Measure items let formula_targets: std::collections::HashSet<&str> = model @@ -152,8 +181,9 @@ pub fn format_md(workbook: &Workbook) -> String { continue; } w!(out, "\n## Category: {}", quote_header(&cat.name)); + // Items are written in insertion order, because that is the order the + // grid's axes consume (gotcha 18). A run of bare items shares a line. let mut bare: Vec = Vec::new(); - let mut grouped: Vec = Vec::new(); for item in cat.items.values() { // For _Measure, skip items that are formula targets // (they'll be recreated from the ## Formulas section) @@ -163,16 +193,19 @@ pub fn format_md(workbook: &Workbook) -> String { continue; } match &item.group { - Some(g) => grouped.push(format!("{}[{}]", quote_name(&item.name), quote_name(g))), None => bare.push(quote_name(&item.name)), + Some(g) => { + if !bare.is_empty() { + w!(out, "- {}", bare.join(", ")); + bare.clear(); + } + w!(out, "- {}[{}]", quote_name(&item.name), quote_name(g)); + } } } if !bare.is_empty() { w!(out, "- {}", bare.join(", ")); } - for g_item in &grouped { - w!(out, "- {g_item}"); - } for g in &cat.groups { if let Some(parent) = &g.parent { w!(out, "> {}[{}]", quote_name(&g.name), quote_name(parent)); @@ -190,7 +223,10 @@ pub fn format_md(workbook: &Workbook) -> String { CellValue::Number(n) => format_number(*n), CellValue::Text(s) | CellValue::Error(s) => pipe_quote(s), }; - w!(out, "{} = {}", coord_str(&key), val_str); + match coord_str(&key) { + coords if coords.is_empty() => w!(out, "= {val_str}"), + coords => w!(out, "{coords} = {val_str}"), + } } } diff --git a/crates/improvise-io/src/persistence/improv.pest b/crates/improvise-io/src/persistence/improv.pest index 8bacdb7c..094086a3 100644 --- a/crates/improvise-io/src/persistence/improv.pest +++ b/crates/improvise-io/src/persistence/improv.pest @@ -24,6 +24,7 @@ initial_view = { "Initial View: " ~ header_name ~ NEWLINE ~ blank_lines } section = _{ category_section | formulas_section + | aggregation_section | data_section | view_section } @@ -69,6 +70,19 @@ formula_line = { formula_target = _{ " "* ~ "[" ~ name ~ "]" } formula_raw = @{ (!(formula_target? ~ NEWLINE) ~ ANY)+ } +// ── Aggregation ────────────────────────────────────────────────────────────── +// +// How a `_Measure` item aggregates over the cells a grid position covers. +// Absent means `sum`, which is `Model::agg_for`'s fallback. + +aggregation_section = { + "## Aggregation" ~ NEWLINE ~ blank_lines ~ + aggregation_line* +} + +aggregation_line = { "- " ~ name ~ ": " ~ agg_kind ~ NEWLINE ~ blank_lines } +agg_kind = @{ "sum" | "avg" | "min" | "max" | "count" } + // ── Data ───────────────────────────────────────────────────────────────────── data_section = { @@ -76,11 +90,14 @@ data_section = { data_line* } +// A cell with NO coordinates is a legal model state — the scalar cell, and the +// placeholder an unfinished records row leaves. It is spelled `= 42`. data_line = { - coord_list ~ " = " ~ cell_value ~ NEWLINE ~ blank_lines + data_coords ~ "= " ~ cell_value ~ NEWLINE ~ blank_lines } -coord_list = { coord ~ (", " ~ coord)* } +data_coords = _{ (coord_list ~ " ")? } +coord_list = { coord ~ (", " ~ coord)* } coord = { name ~ "=" ~ name } cell_value = _{ number | pipe_quoted | bare_value } @@ -98,7 +115,7 @@ view_section = { view_entry* } -view_entry = _{ format_line | hidden_line | collapsed_line | axis_line } +view_entry = _{ format_line | prune_line | hidden_line | collapsed_line | axis_line } axis_line = { name ~ ": " ~ axis_kind ~ (", " ~ name)? ~ NEWLINE ~ blank_lines @@ -107,6 +124,10 @@ axis_line = { axis_kind = @{ "row" | "column" | "page" | "filter" | "none" } format_line = { "format: " ~ rest_of_line ~ NEWLINE ~ blank_lines } +// Absent means false — the compatible reading for a file written before it +// existed, and what the writer emits for a view that does not prune. +prune_line = { "prune: " ~ bool_kw ~ NEWLINE ~ blank_lines } +bool_kw = @{ "true" | "false" } hidden_line = { "hidden: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines } collapsed_line = { "collapsed: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines } diff --git a/crates/improvise-io/src/persistence/parse.rs b/crates/improvise-io/src/persistence/parse.rs index 156cbf90..963ed3b0 100644 --- a/crates/improvise-io/src/persistence/parse.rs +++ b/crates/improvise-io/src/persistence/parse.rs @@ -6,7 +6,7 @@ use pest::Parser; use pest::iterators::{Pair, Pairs}; use pest_derive::Parser; -use crate::formula::parse_formula; +use crate::formula::{AggFunc, parse_formula}; use crate::model::category::Group; use crate::model::cell::{CellKey, CellValue}; use crate::view::Axis; @@ -35,6 +35,7 @@ struct PView { axes: Vec<(String, Axis)>, page_selections: Vec<(String, String)>, format: String, + prune_empty: bool, hidden: Vec<(String, String)>, collapsed: Vec<(String, String)>, } @@ -46,6 +47,7 @@ struct ParsedFile { initial_view: Option, categories: Vec, formulas: Vec<(String, String)>, + measure_agg: Vec<(String, AggFunc)>, data: Vec<(CellKey, CellValue)>, views: Vec, } @@ -177,12 +179,39 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result { Rule::group_hierarchy => { pc.group_parents.push(extract_name_pair(entry)?); } - _ => {} + other => bail!("unhandled rule in category_section: {other:?}"), } } Ok(pc) } +/// The on-disk keyword for an aggregation, exhaustively — the reader's half of +/// `format.rs::agg_keyword`. +fn parse_agg_kind(kind: &str) -> Result { + match kind { + "sum" => Ok(AggFunc::Sum), + "avg" => Ok(AggFunc::Avg), + "min" => Ok(AggFunc::Min), + "max" => Ok(AggFunc::Max), + "count" => Ok(AggFunc::Count), + _ => bail!("Unknown aggregation: {kind}"), + } +} + +/// `## Aggregation`: `- Measure: avg` lines as `(item, AggFunc)` pairs. +fn walk_aggregation_section(pair: Pair<'_, Rule>) -> Result> { + let mut out = Vec::new(); + for line in pair.into_inner() { + if line.as_rule() == Rule::aggregation_line { + let mut parts = line.into_inner(); + let name = extract_name(next(&mut parts, "aggregation_line")?)?; + let kind = next(&mut parts, "aggregation_line")?; + out.push((name, parse_agg_kind(kind.as_str())?)); + } + } + Ok(out) +} + /// `## Formulas`: `(raw text, target category)` pairs. A line with no /// `[Category]` suffix targets `_Measure`. fn walk_formulas_section(pair: Pair<'_, Rule>) -> Result> { @@ -231,21 +260,28 @@ fn walk_data_section(pair: Pair<'_, Rule>) -> Result> for dl in pair.into_inner() { if dl.as_rule() == Rule::data_line { let mut dl_inner = dl.into_inner(); - let coord_list = next(&mut dl_inner, "data_line coords")?; - let value_pair = next(&mut dl_inner, "data_line value")?; - - let coords: Vec<_> = coord_list - .into_inner() - .filter(|p| p.as_rule() == Rule::coord) - .map(extract_name_pair) - .collect::>()?; - + let first = next(&mut dl_inner, "data_line")?; + // `coord_list` is optional in the grammar: a cell with no + // coordinates is the scalar cell, spelled `= 42` (improvise-pnf5). + let (coords, value_pair) = match first.as_rule() { + Rule::coord_list => (walk_coord_list(first)?, next(&mut dl_inner, "data_line")?), + _ => (Vec::new(), first), + }; out.push((CellKey::new(coords), walk_cell_value(value_pair)?)); } } Ok(out) } +/// The `Cat=Item, …` prefix of a data line. +fn walk_coord_list(coord_list: Pair<'_, Rule>) -> Result> { + coord_list + .into_inner() + .filter(|p| p.as_rule() == Rule::coord) + .map(extract_name_pair) + .collect() +} + /// The keyword after `Category:` on an axis line. fn parse_axis_kind(kind_str: &str) -> Result { match kind_str { @@ -281,6 +317,7 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result { axes: Vec::new(), page_selections: Vec::new(), format: String::new(), + prune_empty: false, hidden: Vec::new(), collapsed: Vec::new(), }; @@ -288,9 +325,10 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result { match entry.as_rule() { Rule::axis_line => walk_axis_line(entry, &mut pv)?, Rule::format_line => pv.format = first_str(entry)?, + Rule::prune_line => pv.prune_empty = first_str(entry)? == "true", Rule::hidden_line => pv.hidden.push(extract_name_pair(entry)?), Rule::collapsed_line => pv.collapsed.push(extract_name_pair(entry)?), - _ => {} + other => bail!("unhandled rule in view_section: {other:?}"), } } Ok(pv) @@ -310,9 +348,10 @@ fn walk_file(file: Pair<'_, Rule>) -> Result { } Rule::category_section => parsed.push_category(walk_category_section(pair)?), Rule::formulas_section => parsed.formulas.extend(walk_formulas_section(pair)?), + Rule::aggregation_section => parsed.measure_agg.extend(walk_aggregation_section(pair)?), Rule::data_section => parsed.data.extend(walk_data_section(pair)?), Rule::view_section => parsed.views.push(walk_view_section(pair)?), - _ => {} + other => bail!("unhandled rule in file: {other:?}"), } } Ok(parsed) @@ -367,6 +406,7 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> { if !pv.format.is_empty() { view.number_format = pv.format.clone(); } + view.prune_empty = pv.prune_empty; for (cat, item) in &pv.hidden { view.hide_item(cat, item); } @@ -376,6 +416,16 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> { Ok(()) } +/// `Workbook::new` seeds a `Default` view and `on_category_added` fills it, so +/// a file naming its own views gained a phantom one it never had (improvise-m4rv). +fn drop_unnamed_default(wb: &mut Workbook, named: &[&PView]) { + let default_is_a_phantom = + !named.is_empty() && !named.iter().any(|pv| pv.name == crate::view::DEFAULT_VIEW); + if default_is_a_phantom { + wb.delete_view(crate::view::DEFAULT_VIEW).ok(); + } +} + /// Turn pass 1's collectors into a [`Workbook`]. fn build_workbook(parsed: ParsedFile) -> Result { let name = parsed @@ -389,13 +439,19 @@ fn build_workbook(parsed: ParsedFile) -> Result { // A file written before improvise-60z may carry the scratch views; drop // them, and `initial_view` then falls through to the default. - for pv in parsed + let named: Vec<&PView> = parsed .views .iter() .filter(|pv| !crate::view::VIRTUAL_VIEW_NAMES.contains(&pv.name.as_str())) - { + .collect(); + for pv in &named { build_view(&mut wb, pv)?; } + drop_unnamed_default(&mut wb, &named); + + for (item, agg) in parsed.measure_agg { + wb.model.measure_agg.insert(item, agg); + } if let Some(iv) = &parsed.initial_view && wb.views.contains_key(iv) @@ -420,6 +476,55 @@ pub fn parse_md(text: &str) -> Result { build_workbook(walk_file(parse_tree(text)?)?) } +#[cfg(test)] +mod grammar_coverage { + use super::Rule; + + /// Which walker reads each grammar rule. A new rule in `improv.pest` adds a + /// `Rule` variant and this match stops compiling — improvise-4lgj. + fn walker_of(rule: Rule) -> &'static str { + match rule { + Rule::file | Rule::EOI => "walk_file", + Rule::version_line => "walk_file (skipped: the version is the literal)", + Rule::model_name | Rule::initial_view => "walk_file", + Rule::section => "silent: hoisted into file", + Rule::category_section => "walk_category_section", + Rule::item_list | Rule::grouped_item | Rule::group_hierarchy => "walk_category_section", + Rule::category_entry => "silent: hoisted into category_section", + Rule::formulas_section | Rule::formula_line | Rule::formula_raw => { + "walk_formulas_section" + } + Rule::formula_target => "silent: hoisted into formula_line", + Rule::aggregation_section | Rule::aggregation_line | Rule::agg_kind => { + "walk_aggregation_section" + } + Rule::data_section | Rule::data_line => "walk_data_section", + Rule::data_coords => "silent: hoisted into data_line", + Rule::coord_list | Rule::coord => "walk_coord_list", + Rule::cell_value => "silent: hoisted into data_line", + Rule::number | Rule::bare_value => "walk_cell_value", + Rule::view_section => "walk_view_section", + Rule::axis_line | Rule::axis_kind => "walk_axis_line", + Rule::format_line | Rule::prune_line | Rule::bool_kw => "walk_view_section", + Rule::hidden_line | Rule::collapsed_line => "walk_view_section", + Rule::view_entry => "silent: hoisted into view_section", + Rule::name => "silent: hoisted into its parent", + Rule::pipe_quoted | Rule::pipe_inner | Rule::bare_name => "extract_name", + Rule::header_name | Rule::line_name => "header_name", + Rule::rest_of_line => "first_str", + Rule::blank_lines => "silent: whitespace", + } + } + + /// The compile is the test: this call only type-checks while every `Rule` + /// variant is classified above. The assertions are a smoke check. + #[test] + fn every_grammar_rule_names_the_walker_that_reads_it() { + assert_eq!(walker_of(Rule::prune_line), "walk_view_section"); + assert_eq!(walker_of(Rule::coord_list), "walk_coord_list"); + } +} + #[cfg(test)] mod tests { use super::parse_md; diff --git a/crates/improvise-io/src/persistence/roundtrip_tests.rs b/crates/improvise-io/src/persistence/roundtrip_tests.rs index 9f4f80e1..e85b180c 100644 --- a/crates/improvise-io/src/persistence/roundtrip_tests.rs +++ b/crates/improvise-io/src/persistence/roundtrip_tests.rs @@ -920,3 +920,144 @@ fn a_bare_item_name_with_a_space_is_rejected() { "precondition: the same line quoted must parse" ); } + +// ── prune_empty (improvise-qu4a) ───────────────────────────────────────── + +/// The FLAG is asserted, not the output's stability: both sides dropped it. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn prune_empty_survives_the_roundtrip() { + let mut wb = two_cat_model(); + wb.active_view_mut().prune_empty = true; + let text = format_md(&wb); + assert!(text.contains("prune: true"), "no prune line:\n{text}"); + assert!(parse_md(&text).unwrap().active_view().prune_empty); +} + +/// The direction the default supplies, which is why it is the discriminating +/// one to state separately: a writer that always emits `prune: true` fails it. +#[test] +fn a_view_that_does_not_prune_writes_no_prune_line() { + let mut wb = two_cat_model(); + wb.active_view_mut().prune_empty = false; + let text = format_md(&wb); + assert!(!text.contains("prune:"), "unexpected prune line:\n{text}"); + assert!(!parse_md(&text).unwrap().active_view().prune_empty); +} + +/// An older file carries no `prune:` line; `false` is the compatible reading. +#[test] +fn a_file_without_a_prune_line_does_not_prune() { + let text = "v2025-04-09\n# T\n\n## View: Default\n\n## Category: C\n- a\n"; + assert!(!parse_md(text).unwrap().active_view().prune_empty); +} + +// ── The seeded Default view (improvise-m4rv) ───────────────────────────── + +/// A file that names its own views must not gain the one `Workbook::new` +/// seeds. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_file_naming_its_own_views_gains_no_default() { + let text = "v2025-04-09\n# T\nInitial View: Plan\n\n## View: Plan\n\n## Category: C\n- a\n"; + let wb = parse_md(text).unwrap(); + assert_eq!(wb.views.keys().collect::>(), vec!["Plan"]); + assert_eq!(wb.active_view, "Plan"); +} + +/// The control the fix must not break: a file naming NO view still opens on +/// the seeded one, and a file that names `Default` itself keeps it. +#[test] +fn a_file_naming_no_view_keeps_the_seeded_default() { + let text = "v2025-04-09\n# T\n\n## Category: C\n- a\n"; + let wb = parse_md(text).unwrap(); + assert_eq!(wb.views.keys().collect::>(), vec!["Default"]); + + let named = "v2025-04-09\n# T\n\n## View: Default\n\n## Category: C\n- a\n"; + assert_eq!( + parse_md(named).unwrap().views.keys().collect::>(), + vec!["Default"] + ); +} + +// ── Aggregation (improvise-ppru) ───────────────────────────────────────── + +/// Two saves of one document must agree: `measure_agg` is a `HashMap`, and +/// hash order is not an order (gotcha 18). +#[test] +fn the_aggregation_section_is_written_in_a_stable_order() { + let mut wb = Workbook::new("T"); + for (item, agg) in [ + ("Zebra", crate::formula::AggFunc::Min), + ("Apple", crate::formula::AggFunc::Count), + ("Mango", crate::formula::AggFunc::Avg), + ] { + wb.add_item("_Measure", item).unwrap(); + wb.model.measure_agg.insert(item.into(), agg); + } + let text = format_md(&wb); + let lines: Vec<&str> = text + .lines() + .skip_while(|l| *l != "## Aggregation") + .skip(1) + .take(3) + .collect(); + assert_eq!( + lines, + vec!["- Apple: count", "- Mango: avg", "- Zebra: min"] + ); + assert_eq!(format_md(&parse_md(&text).unwrap()), text); +} + +// ── Interleaved items (improvise-3yv3) ─────────────────────────────────── + +/// The writer emits items in insertion order, splitting a bare run where a +/// grouped item interrupts it. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn an_interleaved_category_keeps_its_item_order() { + let mut wb = Workbook::new("T"); + wb.add_category("C").unwrap(); + for name in ["a", "b"] { + wb.add_item("C", name).unwrap(); + } + wb.add_item_in_group("C", "g1", "G").unwrap(); + wb.add_item("C", "c").unwrap(); + wb.add_item_in_group("C", "g2", "G").unwrap(); + + let text = format_md(&wb); + let body: Vec<&str> = text + .lines() + .skip_while(|l| *l != "## Category: C") + .skip(1) + .take(4) + .collect(); + assert_eq!(body, vec!["- a, b", "- g1[G]", "- c", "- g2[G]"]); + assert_eq!( + parse_md(&text) + .unwrap() + .model + .category("C") + .unwrap() + .ordered_item_names(), + wb.model.category("C").unwrap().ordered_item_names() + ); +} + +// ── The scalar cell (improvise-pnf5) ───────────────────────────────────── + +/// A cell with no coordinates is a legal model state and now has a spelling. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_cell_with_no_coordinates_round_trips() { + use crate::model::cell::CellKey; + let mut wb = two_cat_model(); + wb.model + .set_cell(CellKey::new(vec![]), CellValue::Number(42.0)); + let text = format_md(&wb); + assert!(text.contains("\n= 42\n"), "expected a scalar line:\n{text}"); + let back = parse_md(&text).unwrap(); + assert_eq!( + back.model.data.get(&CellKey::new(vec![])), + Some(&CellValue::Number(42.0)) + ); + assert_eq!(format_md(&back), text); +} diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index b4d839bb..4a908268 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/crates/improvise-protocol/src/basis.rs @@ -48,6 +48,12 @@ pub enum Rejection { /// The batch names a record its basis says nothing about. A well-formed /// client cannot produce this; it closes force-by-omission. RecordUnstated(RecordHash), + /// A record an EARLIER command in the same frame removed. Clause 4's own + /// refusal — `context/code/protocol.md` §5 says why it is not `RecordGone`. + ChainBroken(RecordHash), + /// A mutation in the frame refused to apply at all. The frame is undone; + /// the refusal's own explanation is in the host's status line. + MutationRefused, /// A [`ClientFrame::Save`](crate::ClientFrame::Save) reached a host with /// no path (improvise-368h). Decided before anything runs. @@ -101,7 +107,9 @@ impl Rejection { | Self::LayoutMoved | Self::RecordGone(_) | Self::RecordMoved(_) - | Self::RecordUnstated(_) => true, + | Self::RecordUnstated(_) + | Self::ChainBroken(_) + | Self::MutationRefused => true, Self::NoDocumentPath | Self::SaveFailed => false, } } @@ -126,7 +134,9 @@ impl Rejection { Self::LayoutMoved => Self::RecordGone(sample), Self::RecordGone(_) => Self::RecordMoved(sample), Self::RecordMoved(_) => Self::RecordUnstated(sample), - Self::RecordUnstated(_) => Self::NoDocumentPath, + Self::RecordUnstated(_) => Self::ChainBroken(sample), + Self::ChainBroken(_) => Self::MutationRefused, + Self::MutationRefused => Self::NoDocumentPath, Self::NoDocumentPath => Self::SaveFailed, Self::SaveFailed => Self::DocumentReplaced, } diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index b48f730a..88bed55e 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -396,7 +396,8 @@ pub fn header_level_widths<'a>( for tuple in items { for (level, w) in widest.iter_mut().enumerate() { if let Some(label) = tuple.get(level) { - *w = (*w).max(UnicodeWidthStr::width(label.as_str()) as u16); + let measured = UnicodeWidthStr::width(label.as_str()); + *w = (*w).max(measured.min(MAX_ROW_HEADER_W as usize) as u16); } } } diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 6ec4b26a..2f20be7d 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -21,6 +21,15 @@ fn rejection_message(rejection: &Rejection) -> String { Rejection::RecordUnstated(_) => { "your edit named a row this client had not been told about — it was not applied".into() } + // Clause 4: the whole frame was undone, so the wording is about the + // batch rather than the one command that could not run. + Rejection::ChainBroken(_) => { + "an earlier change in the same batch removed that row — the batch was not applied" + .into() + } + Rejection::MutationRefused => { + "part of that batch was refused — the batch was not applied".into() + } // Not staleness, so neither offers a retry: they are worded as facts // about the host, because that is what they are (improvise-368h). Rejection::NoDocumentPath => { diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index fc4c5f70..8da1d071 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -73,7 +73,7 @@ test.describe("every field the render model projects reaches the screen", () => // The axis line is the shared `view_axis_summary`; its absence would mean // a second derivation had crept in. await expect(page.locator("#views li").nth(0).locator(".axes")).toHaveText( - "R:Product C:Quarter", + "R:Product P:Quarter", ); await page.keyboard.press("Escape"); @@ -196,21 +196,21 @@ test.describe("a pointer event reaches the client and its answer is painted", () // A view row (improvise-iioi). The activation must read the CLICKED index, // not `view_panel_cursor` — so the cursor must not already be on the row - // clicked, or both readings give the same answer. `Default` is index 0 and - // holds the cursor by default, so move to it and click `Plan`. + // clicked, or both readings give the same answer. `Plan` is index 0 and + // holds the cursor by default, so move to it and click `Season`. await page.keyboard.press("Shift+V"); - await expect(page.locator("#views li.selected")).toContainText("Default"); - // Switch to Default by KEYBOARD, leaving the cursor on it. The click then + await expect(page.locator("#views li.selected")).toContainText("Plan"); + // Switch to Plan by KEYBOARD, leaving the cursor on it. The click then // names a different row than the cursor does, which is the whole point. await page.keyboard.press("Enter"); - await expect(page.locator("#view-name")).toHaveText("Default"); - - const plan = page.locator("#views li").filter({ hasText: "Plan" }).first(); - await plan.click(); - // Reading the cursor instead of the index re-selects Default and fails. await expect(page.locator("#view-name")).toHaveText("Plan"); - await expect(page.locator("#views li.active")).toContainText("Plan"); - await expect(page.locator("#views li.selected")).toContainText("Plan"); + + const season = page.locator("#views li").filter({ hasText: "Season" }).first(); + await season.click(); + // Reading the cursor instead of the index re-selects Plan and fails. + await expect(page.locator("#view-name")).toHaveText("Season"); + await expect(page.locator("#views li.active")).toContainText("Season"); + await expect(page.locator("#views li.selected")).toContainText("Season"); // A formula row: navigation only, because FormulaPanel binds no Enter and // a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md. diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index 968d87e3..9fa481d1 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -1,5 +1,5 @@ -//! Report-only bug-hunt reproductions: assert the intended behavior, without fixes. -//! Run with `cargo test --test bug_hunt_reproductions -- --ignored --nocapture`. +//! Bug-hunt reproductions. An `#[ignore]`d one is still open; un-ignoring it is +//! part of the fix. Open ones: `cargo test --test bug_hunt_reproductions -- --ignored`. use improvise_io::import::csv_parser::parse_csv_bytes_with_warnings; use improvise_io::{ @@ -8,7 +8,6 @@ use improvise_io::{ workbook::Workbook, }; #[test] -#[ignore = "improvise-3yv3: known bug reproduction"] fn grouped_item_order_survives_save() { let mut w = Workbook::new("test"); w.add_category("C").unwrap(); @@ -21,7 +20,6 @@ fn grouped_item_order_survives_save() { ); } #[test] -#[ignore = "improvise-ppru: known bug reproduction"] fn aggregation_survives_save() { let mut w = Workbook::new("test"); w.add_item("_Measure", "Price").unwrap(); @@ -34,7 +32,6 @@ fn aggregation_survives_save() { ); } #[test] -#[ignore = "improvise-m4rv: known bug reproduction"] fn deleted_default_stays_deleted() { let mut w = Workbook::new("test"); w.create_view("Custom"); @@ -46,30 +43,37 @@ fn deleted_default_stays_deleted() { ); } #[test] -#[ignore = "improvise-jc95: known bug reproduction"] fn csv_nonfinite_is_not_zero() { for token in ["NaN", "inf", "-inf", "1e999"] { let (r, _) = parse_csv_bytes_with_warnings(format!("Value\n{token}\n").as_bytes()).unwrap(); assert_ne!(r[0]["Value"].as_f64(), Some(0.0), "{token} became zero"); + assert_eq!( + r[0]["Value"].as_str(), + Some(token), + "{token} must survive as its own text, not as some other number" + ); } } #[test] -#[ignore = "improvise-5hfh: known bug reproduction"] fn duplicate_csv_headers_not_silent() { - let result = parse_csv_bytes_with_warnings(b"A,A\n1,2\n"); + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,A\n1,2\n").unwrap(); + let fields = records[0].as_object().unwrap(); + assert_eq!( + fields.len(), + 2, + "both columns must survive as separate fields: {fields:?}" + ); + let values: Vec> = fields.values().map(|v| v.as_f64()).collect(); + assert_eq!(values, [Some(1.0), Some(2.0)]); assert!( - match result { - Err(error) => error.to_string().to_lowercase().contains("duplicate"), - Ok((_, warnings)) => warnings.iter().any(|warning| { - warning.to_lowercase().contains("duplicate") && warning.contains("A") - }), - }, - "duplicate headers silently overwrite a field" + warnings + .iter() + .any(|warning| warning.to_lowercase().contains("duplicate") && warning.contains("A")), + "the rename must be reported: {warnings:?}" ); } #[test] -#[ignore = "improvise-l1f5: known bug reproduction"] fn blank_measure_still_infers_numeric() { use improvise_io::import::{analyzer::FieldKind, wizard::ImportPipeline}; let (records, _) = parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nWest,\n").unwrap(); @@ -78,7 +82,6 @@ fn blank_measure_still_infers_numeric() { assert_eq!(proposal.kind, FieldKind::Measure); } #[test] -#[ignore = "improvise-2cw9: known bug reproduction"] fn repeated_import_coordinates_not_silently_overwritten() { use improvise_io::import::wizard::ImportPipeline; let (records, _) = @@ -86,24 +89,23 @@ fn repeated_import_coordinates_not_silently_overwritten() { let (wb, warnings) = ImportPipeline::new(serde_json::Value::Array(records)) .build_model_with_warnings() .unwrap(); - let total: f64 = wb + let values: Vec = wb .model .data .iter_cells() .filter_map(|(_, v)| v.as_f64()) - .sum(); - assert!( - total == 30.0 - || warnings.iter().any(|warning| { - let warning = warning.to_lowercase(); - (warning.contains("duplicate") - || warning.contains("overwrit") - || warning.contains("overwrot")) - && warning.contains("east") - && warning.contains("revenue") - }), - "total {total}, no warning {warnings:?}" + .collect(); + assert_eq!( + values, + [30.0], + "the two records are one cell and neither may be dropped" ); + let warning = warnings + .iter() + .find(|w| w.to_lowercase().contains("duplicate")) + .unwrap_or_else(|| panic!("summing is a decision and is reported: {warnings:?}")); + assert!(warning.contains("Region=East"), "{warning}"); + assert!(warning.contains("_Measure=Revenue"), "{warning}"); } #[test] #[ignore = "improvise-55v2: known bug reproduction"] @@ -168,25 +170,25 @@ fn label_stays_off_axes_in_new_view() { assert_eq!(w.create_view("Other").axis_of("Description"), Axis::None); } #[test] -#[ignore = "improvise-k8i: known bug reproduction"] fn long_csv_row_is_not_silently_dropped() { - let r = parse_csv_bytes_with_warnings(b"A,B\n1,2,3\n"); + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B\n1,2,3\n4,5\n1,2,3,4\n").unwrap(); + assert_eq!(records.len(), 3, "no row is discarded outright"); + let warning = warnings + .iter() + .find(|w| w.contains("more fields")) + .unwrap_or_else(|| panic!("extra values dropped silently: {warnings:?}")); assert!( - match r { - Err(error) => { - let message = error.to_string().to_lowercase(); - message.contains("3") - && message.contains("2") - && (message.contains("field") || message.contains("column")) - } - Ok((_, warnings)) => warnings.iter().any(|warning| { - let warning = warning.to_lowercase(); - warning.contains("3") - && warning.contains("2") - && (warning.contains("field") || warning.contains("column")) - }), - }, - "extra value 3 dropped silently" + warning.contains("2 row(s)") && warning.contains("2 header"), + "the warning counts the affected rows and the header width: {warning}" + ); + assert!( + warning.contains("widest row: 4"), + "the warning says how much was dropped: {warning}" + ); + let (_, clean) = parse_csv_bytes_with_warnings(b"A,B\n1,2\n").unwrap(); + assert!( + clean.is_empty(), + "control: a full row is not long: {clean:?}" ); } // Passing control: embedded carriage returns already survive persistence. @@ -203,7 +205,6 @@ fn literal_cr_name_survives_roundtrip() { ); } #[test] -#[ignore = "improvise-pnf5: known bug reproduction"] fn scalar_cell_survives_roundtrip() { use improvise_io::model::cell::{CellKey, CellValue}; let mut w = Workbook::new("test"); @@ -232,7 +233,6 @@ fn revenue_book() -> Workbook { w } #[test] -#[ignore = "improvise-s2w3: known bug reproduction"] fn sum_of_derived_measure_agrees_with_identity_expression() { use improvise_io::{formula::parse_formula, model::cell::CellKey}; let mut w = revenue_book(); @@ -249,7 +249,6 @@ fn sum_of_derived_measure_agrees_with_identity_expression() { assert_eq!(a, b); } #[test] -#[ignore = "improvise-s08t: known bug reproduction"] fn unknown_aggregate_reference_is_not_all_measures() { use improvise_io::{ formula::parse_formula, @@ -314,7 +313,6 @@ fn formula_panel_handles_zero_inner_width() { } #[test] -#[ignore = "improvise-pnf5: known bug reproduction"] fn save_with_unfinished_record_is_readable() { use improvise::{ command::{ @@ -337,7 +335,6 @@ fn save_with_unfinished_record_is_readable() { } #[test] -#[ignore = "improvise-7iww: known bug reproduction"] fn circular_formula_not_a_plausible_cached_number() { use improvise_io::{ formula::parse_formula, @@ -363,32 +360,39 @@ fn circular_formula_not_a_plausible_cached_number() { ); } #[test] -#[ignore = "improvise-yae4: known bug reproduction"] fn wide_cell_clamps_without_overflow() { use improvise::{ model::cell::{CellKey, CellValue}, ui::app::App, }; - let mut w = revenue_book(); - w.model.set_cell( - CellKey::new(vec![ - ("Region".into(), "East".into()), - ("_Measure".into(), "Revenue".into()), - ]), - CellValue::Text("x".repeat(65535)), - ); - let app = App::new(w, None); - let widths = improvise::ui::viewport::compute_col_widths( - &app.model_state.workbook.model, - &app.layout, - false, - 0, - ); - assert_eq!( - widths, - vec![improvise::ui::viewport::MAX_COL_WIDTH], - "the Revenue column must reach the width cap" - ); + // 65535 overflows the `+ 1` gap; 65536 truncates the `as u16` cast to 0. + // 20 is the ordinary case the cap must leave alone, gap included. + for (len, expect) in [ + (20usize, 21u16), + (65535, improvise::ui::viewport::MAX_COL_WIDTH), + (65536, improvise::ui::viewport::MAX_COL_WIDTH), + ] { + let mut w = revenue_book(); + w.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "East".into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Text("x".repeat(len)), + ); + let app = App::new(w, None); + let widths = improvise::ui::viewport::compute_col_widths( + &app.model_state.workbook.model, + &app.layout, + false, + 0, + ); + assert_eq!( + widths, + vec![expect], + "a {len}-column value must yield exactly one column, of width {expect}" + ); + } } #[test] fn ordinary_controls() { @@ -417,3 +421,25 @@ fn ordinary_controls() { ); assert!(parse_md(&format_md(&w)).is_ok()); } + +// Passing control (improvise-nvq4): quote_name already pipe-quotes a name +// containing `/`, so hidden/collapsed round-trip. The report did not reproduce. +#[test] +fn slash_in_hidden_name_survives_roundtrip() { + let mut w = Workbook::new("test"); + w.add_category("C").unwrap(); + w.add_item("C", "A/B").unwrap(); + w.add_item_in_group("C", "x", "G/H").unwrap(); + w.active_view_mut().hide_item("C", "A/B"); + w.active_view_mut().toggle_group_collapse("C", "G/H"); + let text = format_md(&w); + let r = parse_md(&text).unwrap_or_else(|e| panic!("unparseable: {e}\n{text}")); + assert!( + r.active_view().is_hidden("C", "A/B"), + "hidden lost:\n{text}" + ); + assert!( + r.active_view().is_group_collapsed("C", "G/H"), + "collapsed lost:\n{text}" + ); +} diff --git a/web/demo.improv b/web/demo.improv index b2eb845b..b1e14a28 100644 --- a/web/demo.improv +++ b/web/demo.improv @@ -10,6 +10,14 @@ _Index: none _Dim: none format: ,.0 +## View: Season +Quarter: row +_Measure: column +Product: page, Latte +_Index: none +_Dim: none +format: ,.0 + ## Formulas - Profit = Revenue - Cost