fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'main' into wave/n-browser-boot

Browse Source
This commit is contained in:
Edward Langley
2026-09-21 14:51:37 -07:00
parent 12714c7527 79da33859a
commit a55e7dd5b7
29 changed files with 2380 additions and 218 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/core.md
+45 -2
View File
@@ -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(<expr>)` asks the
expression of each member, and an expression that cannot be evaluated is an
context/test-evidence/command.md
+123 -7
View File
@@ -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`
context/test-evidence/core.md
+258
View File
@@ -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 <cat> <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
context/test-evidence/formula-and-io.md
+348 -3
View File
@@ -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,
crates/improvise-command/src/ui/app.rs
+34
View File
@@ -342,6 +342,16 @@ pub struct App {
pub storage: Box<dyn crate::storage::Storage>,
}
/// 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<PathBuf>) -> 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 {
crates/improvise-command/src/ui/host.rs
+158 -15
View File
@@ -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<Addressed> {
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<Addressed> {
@@ -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"
);
}
crates/improvise-command/src/ui/project.rs
+80 -4
View File
@@ -674,7 +674,7 @@ pub fn grid_updates(mirror: &GridCache, desired: &GridCache) -> Vec<GridUpdate>
/// 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<SessionId, ClientSession>,
@@ -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]
crates/improvise-command/src/ui/reduce.rs
+32 -3
View File
@@ -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<Checked<'a>>);
pub struct CheckedBatch<'a> {
checked: Vec<Checked<'a>>,
/// 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<Basis>,
}
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<Self, Rejection> {
@@ -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<Rejection> {
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<Checked<'a>>;
fn into_iter(self) -> Self::IntoIter {
self.0.into_iter()
self.checked.into_iter()
}
}
crates/improvise-command/src/ui/viewport.rs
+9 -5
View File
@@ -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<u16> {
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);
}
}
crates/improvise-core/src/format.rs
+19 -2
View File
@@ -25,10 +25,16 @@ pub fn parse_number_format(fmt: &str) -> (bool, u8) {
(comma, precision.parse::<u8>().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");
}
}
crates/improvise-core/src/model/types.rs
+204 -20
View File
@@ -63,24 +63,36 @@ fn eval_agg(
eval_at: &dyn Fn(&Expr, &CellKey) -> Result<f64, String>,
) -> Result<f64, String> {
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<f64> = match inner {
let values: Vec<f64> = 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<String>) -> Result<CategoryId> {
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<CellKey> = 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]
crates/improvise-core/src/view/layout.rs
+126 -8
View File
@@ -126,27 +126,52 @@ fn data_index_to_visual(items: &[AxisEntry], data_idx: usize) -> Option<usize> {
.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<AxisEntry>,
pending: Option<AxisEntry>,
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<AxisEntry>, keep: impl Fn(usize) -> bool) {
let mut kept = Vec::new();
let mut kept: Vec<AxisEntry> = Vec::new();
let mut pending_header: Option<AxisEntry> = 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<AxisEntry>, 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"
);
}
}
crates/improvise-core/src/view/mod.rs
+1 -1
View File
@@ -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};
crates/improvise-core/src/view/types.rs
+68
View File
@@ -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");
crates/improvise-core/src/workbook.rs
+27 -3
View File
@@ -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<String>) -> 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");
crates/improvise-formula/src/parser.rs
+42 -5
View File
@@ -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();
crates/improvise-io/src/import/analyzer.rs
+68 -10
View File
@@ -64,10 +64,10 @@ pub fn detect_date_format(samples: &[&str]) -> Option<String> {
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<FieldProposal> {
.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<FieldProposal> {
// 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<String> = values
let distinct_vec: Vec<String> = 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<String> = (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<Value> = 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<String> = (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<Value> = 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<Value> = 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<Value> = 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);
}
}
crates/improvise-io/src/import/csv_parser.rs
+133 -13
View File
@@ -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<Value>, Vec<St
let mut records = Vec::new();
let mut headers: Vec<String> = Vec::new();
let mut header_warnings: Vec<String> = Vec::new();
if has_headers {
headers = reader
let raw: Vec<String> = 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<Value>, Vec<St
}
}
let warnings = headers
.iter()
.zip(&missing_counts)
.filter(|&(_, &count)| count > 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<String>) -> (Vec<String>, Vec<String>) {
let mut claimed: HashSet<String> = raw.iter().cloned().collect();
let mut seen: HashSet<String> = 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<Path>]) -> Result<Vec<Value>> {
@@ -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::<f64>() {
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::<f64>()
&& 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::<f64>().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
crates/improvise-io/src/import/wizard.rs
+136 -5
View File
@@ -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<PendingCell> = vec![];
let mut cell_index: HashMap<CellKey, usize> = 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<String> {
let (first, rest) = collisions.split_at(collisions.len().min(3));
let mut details: Vec<String> = 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<String> {
@@ -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<String> = 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);
}
}
crates/improvise-io/src/persistence/format.rs
+42 -6
View File
@@ -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<String> = Vec::new();
let mut grouped: Vec<String> = 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}"),
}
}
}
crates/improvise-io/src/persistence/improv.pest
+24 -3
View File
@@ -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 }
crates/improvise-io/src/persistence/parse.rs
+120 -15
View File
@@ -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<String>,
categories: Vec<PCategory>,
formulas: Vec<(String, String)>,
measure_agg: Vec<(String, AggFunc)>,
data: Vec<(CellKey, CellValue)>,
views: Vec<PView>,
}
@@ -177,12 +179,39 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result<PCategory> {
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<AggFunc> {
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<Vec<(String, AggFunc)>> {
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<Vec<(String, String)>> {
@@ -231,21 +260,28 @@ fn walk_data_section(pair: Pair<'_, Rule>) -> Result<Vec<(CellKey, CellValue)>>
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::<Result<_>>()?;
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<Vec<(String, String)>> {
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<Axis> {
match kind_str {
@@ -281,6 +317,7 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result<PView> {
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<PView> {
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<ParsedFile> {
}
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<Workbook> {
let name = parsed
@@ -389,13 +439,19 @@ fn build_workbook(parsed: ParsedFile) -> Result<Workbook> {
// 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<Workbook> {
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;
crates/improvise-io/src/persistence/roundtrip_tests.rs
+141
View File
@@ -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<_>>(), 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<_>>(), 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<_>>(),
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);
}
crates/improvise-protocol/src/basis.rs
+12 -2
View File
@@ -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,
}
crates/improvise-protocol/src/render.rs
+2 -1
View File
@@ -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);
}
}
}
crates/improvise-wasm-client/src/socket.rs
+9
View File
@@ -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 => {
harness/tests/contract.spec.js
+12 -12
View File
@@ -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.
tests/bug_hunt_reproductions.rs
+99 -73
View File
@@ -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<Option<f64>> = 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<f64> = 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}"
);
}
web/demo.improv
+8
View File
@@ -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