Merge gh/main (4b88084) into wire2/2wu3
Resolved context/test-evidence/clients.md: gh/main's version, whose improvise-w7qd/5kdr paragraphs continue the entry above the conflict, followed by this branch's two improvise-0j0f/4img entries. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Edward Langley
+3
-1
@@ -40,7 +40,9 @@ improvise-command = { path = "crates/improvise-command" }
|
||||
ratatui = "0.30"
|
||||
crossterm = "0.29"
|
||||
serde = { version = "1", features = ["derive"] }
|
||||
serde_json = "1"
|
||||
# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate,
|
||||
# because each build stands alone (improvise-6yvg, context/code/protocol.md).
|
||||
serde_json = { version = "1", features = ["float_roundtrip"] }
|
||||
anyhow = "1"
|
||||
thiserror = "2"
|
||||
indexmap = { version = "2", features = ["serde"] }
|
||||
|
||||
+133
-4
@@ -935,6 +935,52 @@ There is deliberately **no `dirty = true`** in it: it declares
|
||||
of `MarkDirty` — harmless, and a second home for the one thing that commit set
|
||||
out to give exactly one.
|
||||
|
||||
### `RecordRewrite` — a row's edits composed, not replayed (improvise-uo2p)
|
||||
|
||||
A staged drill edit is a statement about the record **as the snapshot showed
|
||||
it**. It is not a statement about the model as the previous edit left it. The
|
||||
edits in one row do not commute when they are replayed one at a time. A rename
|
||||
moves every cell in the row (gotcha 1). A measure edit replayed after it writes
|
||||
the old key, which leaves a ghost record there and a stale value in the moved
|
||||
record. A second rename replayed after the first finds nothing at the old keys,
|
||||
so it is dropped. `ApplyAndClearDrill` replayed them in `HashMap` order, so the
|
||||
outcome was different from one App to the next. That is gotcha 18's class of
|
||||
bug, found in an effect rather than an index.
|
||||
|
||||
**Sorting is not the fix.** "Measures before renames" repairs the ticket's own
|
||||
case and still drops a second rename every time (rival 1 in the evidence). Any
|
||||
fixed order makes the corruption reproducible instead of removing it. So the
|
||||
rule is **build each row's final record once, then write it**:
|
||||
|
||||
- `RecordRewrite::of(model, row, edits)` collects the row's renames and measure
|
||||
edits into maps keyed by column, and reads each unedited value off the model
|
||||
**before anything is written**. It then states the rewrite: clear every old
|
||||
key, and write every cell at the key with *all* renames applied. A measure
|
||||
edit that is staged replaces the carried value. The result does not depend on
|
||||
the order of the edits. A drill stages at most one edit per `(row, column)`,
|
||||
so no two edits in a row name the same column. A blank rename is refused, and
|
||||
the row's other edits still land at the unrenamed coordinates, as before.
|
||||
- `RecordRewrite::merged` concatenates the rows' rewrites **by phase**: every
|
||||
refusal, then every item added, then every clear, then every write. The clears
|
||||
come first so that a swap is a swap. Rows are taken in snapshot row order
|
||||
(`BTreeMap`), so where two final records land on one cell, **the later row
|
||||
wins**. That is the answer plain records mode gives for the same renames made
|
||||
top to bottom.
|
||||
|
||||
The collision rule is a **choice**, and it still loses the earlier row's values
|
||||
silently. Plain records mode loses them the same way when a rename lands on an
|
||||
existing record. Refusing the colliding rows instead would be a change to both
|
||||
paths.
|
||||
|
||||
**One copy.** The plain records path (`commit.rs::commit_plain_records_edit`)
|
||||
calls the same `RecordRewrite::of` with its one edit, so `rename_record_coordinate`
|
||||
and the drill's `apply_coordinate_rename`/`apply_measure_edit` are gone. This is
|
||||
the consolidation improvise-fdk1 asked for, "regardless" of its wire half. For a
|
||||
single edit the output is the effects the plain path used to emit, reordered from
|
||||
`clear, set` per cell to all clears then all sets, which touches distinct keys.
|
||||
One behaviour moved on the drill side: it used to discard `add_item`'s error, and
|
||||
now goes through `effect::AddItem`, which refuses when the category has gone.
|
||||
|
||||
### `Save` / `SaveAs` abort the batch (improvise-8tk / improvise-lae)
|
||||
|
||||
A failed save aborts the rest of the batch, so the `Quit` that `wq`/`ZZ` chains
|
||||
@@ -2629,8 +2675,10 @@ At bottom-right `EnterAdvance` has no place to go, so it emits a single `AbortCh
|
||||
stages in drill pending edits while in drill mode, or applies directly in plain
|
||||
records mode; for real keys it writes to the model.
|
||||
|
||||
`rename_record_coordinate` renames one coordinate of a records row. In wide format a
|
||||
row is a group of cells — one per measure — so the rename has to move **every** one
|
||||
`commit_plain_records_edit` hands a coordinate or measure edit to
|
||||
`effect::RecordRewrite::of`, the one home it shares with the drill (improvise-uo2p;
|
||||
see `RecordRewrite` above). It replaced `rename_record_coordinate`. In wide format a
|
||||
row is a group of cells — one per measure — so a rename has to move **every** one
|
||||
of them, not just the cell under the cursor (improvise-c5v, gotcha 1). Every cell in
|
||||
the row moves, and a row seeded by `add-record-row` has no measure cells yet: its
|
||||
coordinates live on a lone measure-less cell, which must move too or the row loses
|
||||
@@ -3187,8 +3235,8 @@ the cursor**. The result was silent corruption of an unrelated record: appending
|
||||
a row changes `view_frame`'s hash, the frame observation inside `apply_effects`
|
||||
invalidates the cursor to the origin *mid-batch* (gotcha 24), and
|
||||
`EnterEditAtCursor` then pre-fills the editor with row 0's category coordinate.
|
||||
Enter commits through `rename_record_coordinate`, which by gotcha 1 moves every
|
||||
measure cell of that row.
|
||||
Enter commits through `rename_record_coordinate` (since improvise-uo2p,
|
||||
`RecordRewrite::of`), which by gotcha 1 moves every measure cell of that row.
|
||||
|
||||
Two things are worth carrying past this fix.
|
||||
|
||||
@@ -3262,3 +3310,84 @@ key the two questions agree, because a cursor key never carries a None-axis
|
||||
category. The **collapsed-group fold** half of improvise-l4mp is not covered:
|
||||
its key carries the group's name as an ordinary coordinate, so no question about
|
||||
the key alone can see it — it needs `GridLayout::folds_a_group`.
|
||||
|
||||
## A command is a function of its context — the determinism law (improvise-hzjo)
|
||||
|
||||
`cmd/determinism.rs` holds the law the old `law_toggle_group_involution` claimed
|
||||
and could not state. For **every** registered command and every argument list
|
||||
that builds it, the effects `execute` returns must be equal, by `Debug` text,
|
||||
across contexts built **independently from equal data**: four `App`s, each
|
||||
parsed from `web/demo.improv` and given the same buffers, expanded categories,
|
||||
yank, panels and cursor.
|
||||
|
||||
**Why the instances must be distinct.** The old test called `execute` twice
|
||||
against one `CmdContext`. Every `HashMap` it reached was the same instance with
|
||||
the same `RandomState`, so it iterated the same way both times: `f(x) == f(x)`,
|
||||
which nothing can fail. Separately built `App`s give every map its own seed. So a
|
||||
command whose effects follow hash iteration order disagrees with itself. Hash
|
||||
order is the one kind of nondeterminism this repo has actually shipped (gotcha
|
||||
18). This law can see it where a command's `execute` builds it, as the drill
|
||||
snapshot's record order was. It cannot see the other shipped instance, the
|
||||
analyzer's `HashSet`: `analyze_records` is reached only from inside
|
||||
`Effect::apply`, which is outside this law (see below). The companion test
|
||||
`only_distinct_instances_can_see_a_command_that_follows_hash_order` keeps that
|
||||
property checked: a test-only command that emits `ctx.buffers` in iteration
|
||||
order must agree with itself on one context and disagree across two.
|
||||
|
||||
**Four instances, not two.** A rival that takes *the first* entry of a map, not
|
||||
all of them, agrees across two seeds whenever both happen to put the same entry
|
||||
first. With 21 buffers that is roughly one chance in 21, and three comparisons
|
||||
make it negligible. The fixture's values are also chosen to stay distinct under
|
||||
the edits commands make. A first version used `v0`…`v15`, which `pop()` collapses
|
||||
to `v`, and so it let `command-mode-backspace` reading an arbitrary buffer pass.
|
||||
|
||||
**The store needs several records to order, so the drills come from the
|
||||
document (improvise-e1sm).** The registry's `candidate_args` are extended with
|
||||
names the demo really has, because coordinates that match nothing never reach
|
||||
the records a drill or a fit would order. A hand-picked name is not enough. The
|
||||
first version's only real drill was `Product/Latte, Quarter/Q2`: one record, two
|
||||
cells. With the store's sort removed, two contexts put those two cells in the
|
||||
same order about half the time, and with three comparisons the law passed about
|
||||
one run in eight (measured: 46 of 400). So the arguments now include **every
|
||||
item of every regular category as a one-coordinate drill**, read off the parsed
|
||||
document. A one-coordinate drill spans every other category whole, so it matches
|
||||
the whole slice of records under that item, not one record picked by name. In
|
||||
the demo that is four records and eight cells for each of the eight drills. The
|
||||
test
|
||||
also asserts that the widest candidate it actually passes matches at least four
|
||||
records. That guard catches a demo with too little data, and it catches the
|
||||
derived drills being dropped from the argument list. It does **not** catch the
|
||||
comparison losing its wide drills some other way, because it measures what the
|
||||
argument lists match, not what the built commands ordered (improvise-b9f2). The
|
||||
review showed this with a rival: a `drill-into-cell` parser that refuses fewer
|
||||
than two arguments sends every one-coordinate drill to the one-record cursor
|
||||
fallback. The guard still reports four records, and with the store's sort
|
||||
removed the law passed 38 of 300 runs. With the store's sort removed and the
|
||||
parser intact, the law fails 400 of 400 runs.
|
||||
|
||||
**What it cannot see.** Only commands and arguments the fixture reaches. A
|
||||
command whose nondeterminism needs some other state, such as records mode, a
|
||||
wizard or a drill in progress, is outside this fixture. And `Debug` equality is
|
||||
text equality: two effects that differ only in something their `Debug` omits are
|
||||
equal here.
|
||||
|
||||
**Its scope is what `execute` returns, not what `Effect::apply` does.** The law
|
||||
compares the effects a command hands back. Many effects are unit structs that
|
||||
decide all their work inside `apply`: `ApplyAndClearDrill`,
|
||||
`SortData`, `CleanEmptyRecords`, `Save`, `RebuildLayout`, `AcceptWizardModel`.
|
||||
Hash-order iteration inside an `apply` is therefore invisible here, however
|
||||
often the law runs. improvise-uo2p is the worked example.
|
||||
`ApplyAndClearDrill::apply` walks `drill.pending_edits`, a `HashMap`, in hash
|
||||
order. A measure edit and a category rename staged on one row can then write a
|
||||
ghost record. The command that emits it passes this law, because the effect's
|
||||
`Debug` is just its name. So "the one kind of nondeterminism this repo has
|
||||
shipped" is guarded here only on the `execute` side. The `apply` side needs its
|
||||
own test per effect, or a law over `App` state after `apply_effects`.
|
||||
|
||||
**The exemption list.** `OUTSIDE_THE_CONTEXT` names commands that legitimately
|
||||
depend on something outside the context, such as a clock or an entropy source,
|
||||
each with its reason. It is empty: `fit-measure`'s seed is an argument with a
|
||||
fixed default, not a clock read. The list is checked in both directions. An
|
||||
unlisted command that diverges fails the test, and so does a listed one that
|
||||
does not diverge. A stale entry therefore cannot linger, and a new registration
|
||||
is covered the day it lands.
|
||||
|
||||
+46
-20
@@ -756,28 +756,54 @@ coming back one ULP off, both in `CellDisplay::value` (host to client, so `yy`
|
||||
copied a neighbour) and in a `SetCell` (client to host, so a typed or pasted
|
||||
number landed as a neighbour).
|
||||
|
||||
The feature is on for **every crate that parses a frame**: `improvise-ws-server`
|
||||
(`transport.rs`), `improvise-worker-server` (`worker.rs`) and
|
||||
`improvise-wasm-client` (`socket.rs`). Each crate's own manifest names it,
|
||||
because each browser bundle is built from one crate alone. The `wasm` CI job
|
||||
and `wasm-pack` both invoke per crate, and feature unification cannot reach
|
||||
across separate invocations (`context/repo-map.md`, `## Wasm`). Enabling it in
|
||||
one crate and relying on the others to inherit it works in `cargo test` and
|
||||
nowhere else.
|
||||
The feature is on for **every workspace crate with a normal dependency on
|
||||
serde_json** (improvise-dz8c). Each crate's own manifest names it, because each
|
||||
build is made from one crate alone: the `wasm` CI job and `wasm-pack` invoke
|
||||
per crate, and feature unification cannot reach across separate invocations
|
||||
(`context/repo-map.md`, `## Wasm`). Enabling it in one crate and relying on the
|
||||
others to inherit it works in `cargo test` and nowhere else.
|
||||
|
||||
That is also why the end-to-end tests cannot guard this alone. They run in one
|
||||
unified build with `improvise-ws-server` as a dev-dependency, so they stay
|
||||
green with the feature removed from the browser bundle.
|
||||
`every_build_that_parses_a_frame_enables_float_roundtrip` asks `cargo tree`
|
||||
per crate over normal edges instead. A new crate that parses frames belongs in
|
||||
its list.
|
||||
**Why "can parse" and not "parses a frame".** The first version of the rule
|
||||
covered only the three crates that parse frames and listed them by hand.
|
||||
Frames are not the only place a user's number crosses a JSON parser.
|
||||
`improvise-io`'s legacy-JSON `.improv` load, `improvise-command`'s JSON import
|
||||
(`StartImportWizard`, `ImportJsonHeadless`) and the root binary's
|
||||
`improvise import` all parsed with the lossy default, and in the TUI binary
|
||||
nothing enabled the feature (improvise-6yvg, reproduced: `0.24815714285714285`
|
||||
stored as `…283`). The worker bundle happened to be exact through unification,
|
||||
so one import gave different numbers in the two front ends. "Parses JSON input"
|
||||
cannot be read off a manifest, but "has a normal serde_json dependency" can, and
|
||||
it over-approximates in the cheap direction: the feature changes only parsing,
|
||||
so a crate that only serializes pays nothing for it. Dev-dependencies are out,
|
||||
because a test build ships nowhere. That also leaves out `gp-tree`, whose only
|
||||
serde_json is a dev-dependency (its example CLI reads a saved model). Every
|
||||
number it persists is an `f32`, and 3M random `f32`s all read back exactly
|
||||
through the lossy parser, against about 30% of random `f64`s.
|
||||
|
||||
Not covered, and filed separately rather than widened into this change
|
||||
(improvise-6yvg): `improvise-io`'s JSON import and `improvise-command`'s JSON
|
||||
reads also parse numbers with the default parser. They are file formats, not
|
||||
frames. In the TUI binary nothing enables the feature. In the worker bundle
|
||||
unification happens to reach them, so one import can give different numbers
|
||||
in the two front ends.
|
||||
**Why a declaration and not the build graph.** The first check asked
|
||||
`cargo tree` whether each listed crate's *build* had the feature. That answer
|
||||
can be yes by accident. `improvise-command` depends on `improvise-io`, so with
|
||||
the feature removed from command's manifest, command's own test build still
|
||||
has it, and its import test stays green. The rule requires the crate's own
|
||||
manifest to say it, so a later change to a dependency cannot take it away.
|
||||
|
||||
**Why not enable it once, in `improvise-protocol`.** That removes the list, but
|
||||
it would give the protocol a normal serde_json dependency it has no use for
|
||||
(its repo-map entry says core and serde only). It would also reach only builds
|
||||
that include the protocol, and `improvise-io` does not depend on it. Under this
|
||||
rule the protocol's one serde_json dependency, the optional `test-support` one,
|
||||
carries the feature too. That is off in every shipped build, so it costs
|
||||
nothing.
|
||||
|
||||
`every_crate_that_can_parse_json_enables_float_roundtrip` (`tests/json_numbers.rs`)
|
||||
reads `cargo metadata --no-deps` and names every package, and every
|
||||
target-specific entry, whose normal serde_json dependency lacks the feature. A
|
||||
new crate is covered the day it declares the dependency. It does not reach
|
||||
`harness/opfs-contract`, which is its own workspace and only serializes.
|
||||
|
||||
The end-to-end tests cannot guard this alone. They run in one unified build
|
||||
with `improvise-ws-server` as a dev-dependency, so they stay green with the
|
||||
feature removed from the browser bundle.
|
||||
|
||||
### `AxisFrame` — the grid's frame, and why it is *one* type
|
||||
|
||||
|
||||
@@ -4626,24 +4626,12 @@ before #178 too, as the review noted.
|
||||
**Fault injected:** the same pre-fix tree. Fails: the host stored
|
||||
`4598108823619341467` in South/Cost.
|
||||
|
||||
### `every_build_that_parses_a_frame_enables_float_roundtrip`
|
||||
|
||||
The two tests above run in **one** feature-unified test build, which includes
|
||||
`improvise-ws-server` and `improvise-command` as dev-dependencies. So they pass
|
||||
whenever **any** crate in that build enables the feature. The shipped browser
|
||||
bundle is built from one crate alone, so it would still be lossy. That was
|
||||
demonstrated: with the feature removed from `improvise-wasm-client` only, both
|
||||
tests stayed green while `cargo tree` showed no `float_roundtrip` in the wasm
|
||||
bundle. This test asks cargo instead, once per crate that parses a frame:
|
||||
`cargo tree --locked --edges normal,features --invert serde_json --package
|
||||
<crate>`, with `--target wasm32-unknown-unknown` for the two bundles. It uses
|
||||
normal edges only, because a dev edge would unify the feature back in
|
||||
(`context/repo-map.md`, `## Wasm`).
|
||||
|
||||
**Faults injected:** the feature removed from `improvise-wasm-client` alone,
|
||||
which fails naming that crate while the two tests above pass. Then removed from
|
||||
`improvise-ws-server` alone, which fails naming it. The native path runs
|
||||
without `--target`, so the two arms are different invocations.
|
||||
**Retired: `every_build_that_parses_a_frame_enables_float_roundtrip`.** It asked
|
||||
`cargo tree` about three crates listed by hand. improvise-dz8c replaced it with
|
||||
`every_crate_that_can_parse_json_enables_float_roundtrip`, whose entry is in
|
||||
`tooling.md`. The injections recorded for it still hold: with the feature
|
||||
removed from `improvise-wasm-client` alone, the two tests above stay green, so
|
||||
they cannot guard the bundle alone.
|
||||
|
||||
### resolve::tests::every_grid_move_lands_where_its_binding_says
|
||||
|
||||
@@ -4677,6 +4665,43 @@ laundering invisible (the edge) is also the one that makes a wrong-direction
|
||||
jump visible, so moving off the edge fixed one blind spot and opened the other,
|
||||
and each needs its own question.
|
||||
|
||||
**improvise-w7qd, and the floor was the hole.** As first written the test
|
||||
covered `move-selection` and a four-entry jump table, behind `pressed >= 14`.
|
||||
Two things passed the whole workspace, both found in re-review of #180 at
|
||||
544744c: negating the `scroll-rows` and `page-scroll` arms, so `Ctrl-d`/`Ctrl-u`
|
||||
and `PgDn`/`PgUp` all scroll the wrong way (advertised, and still "acting" from
|
||||
the centre); and dropping the jump table *with* `$` sent to the first column,
|
||||
because the sixteen `move-selection` sequences met the floor on their own. The
|
||||
test now walks **every** command Normal binds. `scroll-rows` and `page-scroll`
|
||||
state their landing from their own argument exactly as `move-selection` does —
|
||||
from the centre of a 3x3 grid any step of at least one lands at `1 + d`
|
||||
clamped, so a sign is visible whatever the page size — and a command that moves
|
||||
the cursor (in the same view) without a stated landing **fails** rather than
|
||||
being skipped. So a dropped table entry is caught by derivation, not by a count:
|
||||
under the second rival it fails naming every jump as moving with nothing stating
|
||||
where. The first rival fails on `PgUp` (`page-scroll ["-1"]`) and, alone, on
|
||||
`Ctrl-u` (`scroll-rows ["-5"]`). A last assertion that each kind was pressed at
|
||||
least once guards the other direction — a binding vanishing — and is a named
|
||||
list on purpose, since that is the one thing no walk over bindings can see.
|
||||
|
||||
**improvise-5kdr: the 3x3 fixture saw only a sign.** From the centre of a 3x3
|
||||
grid every step of one or more clamps to an edge, so the w7qd version stated
|
||||
`page-scroll` as `1 + d` — one row, which is not what it does — and passed
|
||||
because the true landing clamped to the same edge. Measured on #198 at 8772db2:
|
||||
`page-scroll` ignoring the page size together with `scroll-rows` moving one row,
|
||||
or `move-selection` moving `2d`, left the test green. The walk now stands at
|
||||
(10,10) of a 21x21 grid with an 8x8 window, far from every edge, so a landing
|
||||
shows magnitude: `10+d` for `move-selection` and `scroll-rows`, `10 + page·d`
|
||||
for `page-scroll`, 0/20 for the jumps. The page size is not written into the
|
||||
test: `grid_move`'s page rule moved into `page_rows(window_rows)` and the test
|
||||
asks it, so the rule has one home. Rivals, each run: `move-selection` doubled
|
||||
fails on `Up` (`move-selection ["-1","0"]`); the page-size-and-one-row pair
|
||||
fails on `PgUp` (`page-scroll ["-1"]`); the one-row `scroll-rows` alone fails on
|
||||
`Ctrl-u` (`scroll-rows ["-5"]`); the jump-table rival still fails naming every
|
||||
jump. The residual, stated: because the test reads `page_rows`, a change to the
|
||||
page rule itself moves the test with it — whether that rule is the TUI's is
|
||||
`tests/lockstep.rs`'s question, not this one's.
|
||||
|
||||
### an_edit_is_refused_when_the_cell_was_written_while_the_editor_was_open
|
||||
|
||||
improvise-0j0f, found by the review of PR #188 at 48f52e7 and confirmed
|
||||
|
||||
@@ -5314,6 +5314,63 @@ before `refuse_and_stop` was applied to its arms. For `load`, the injected fault
|
||||
is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test
|
||||
then fails with "`load absent.improv . q` quit having loaded nothing".
|
||||
|
||||
### ui::effect::tests::a_drill_rows_measure_edits_follow_its_rename
|
||||
|
||||
improvise-uo2p. `ApplyAndClearDrill` walked `pending_edits`, a `HashMap`, and
|
||||
applied each edit alone against the model as the previous one left it. A rename
|
||||
moves the row's cells. A measure edit then wrote `row.cell_key(measure)`, the
|
||||
snapshot's old key. So when the rename came first, a ghost record appeared at the
|
||||
old coordinates carrying the new value, and the moved record kept the stale one.
|
||||
The reviewer saw this in 16 of 40 fresh Apps. The fixture stages two measure edits
|
||||
and a rename on drill row 1, so a wrong answer needs only one of the three orders
|
||||
that put the rename last. Each assertion runs over 32 fresh Apps and compares the
|
||||
**whole** store, so the ghost and the stale value are both visible. Before the
|
||||
fix it failed on five of five runs. Rival 3 (the rewrite addresses measure edits
|
||||
at the unrenamed key) fails it.
|
||||
|
||||
### ui::effect::tests::a_drill_row_renamed_twice_and_edited_lands_as_one_record
|
||||
|
||||
improvise-uo2p. This is the case that fails **every** time, not with some
|
||||
probability. Each rename read the snapshot's old keys. The second rename found
|
||||
nothing there, because the first had moved it, so one rename was always dropped
|
||||
and the row split into a `Clothing/Feb` record and a `Drink/Jan` record. Rival 1
|
||||
is the ticket's proposed fix direction: measures before renames, each edit
|
||||
applied alone. It passes the ticket's own case and fails here. So "a fixed
|
||||
order" is not a fix; the renames have to be composed.
|
||||
|
||||
### ui::effect::tests::a_drill_that_swaps_two_records_coordinates_swaps_them
|
||||
|
||||
improvise-uo2p. Row 0 is renamed onto row 1's coordinates and row 1 onto row 0's.
|
||||
If the rows are applied in sequence, the first move overwrites the other record
|
||||
and the second move carries the overwritten values back, so one record's data
|
||||
is lost in either order. Rival 2 composes each row correctly but writes it
|
||||
before reading the next row, and it fails here and nowhere else. That is why
|
||||
`RecordRewrite::merged` reads every row off the model first, then clears every
|
||||
old key, then writes.
|
||||
|
||||
### ui::effect::tests::a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates
|
||||
|
||||
improvise-uo2p. Row 1's staged `Cost` belongs to row 1's record, even though row
|
||||
0 is being renamed away from coordinates and row 1 onto them. Before the fix,
|
||||
some hash orders left `Clothing/Jan/Cost = 999` as a ghost. Rival 3 fails it.
|
||||
|
||||
### ui::effect::tests::two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row
|
||||
|
||||
improvise-uo2p. Two rows renamed onto the same coordinates collide per cell. The
|
||||
rule chosen is that the later snapshot row wins, which is what plain records
|
||||
mode gives when the same renames are made top to bottom. Before the fix the
|
||||
winner was the hash order's choice. Rival 4 reverses the row order in `merged`
|
||||
and fails here. The rule is a choice, and it still loses data silently. See
|
||||
`context/code/command.md`, `RecordRewrite`.
|
||||
|
||||
### ui::effect::tests::a_record_rewrite_is_the_same_under_every_order_of_its_edits
|
||||
|
||||
improvise-uo2p. The composition is order-independent **by construction**, so it
|
||||
is asserted over all 24 orders of four edits on one row: two renames, a measure
|
||||
write and a measure clear. The test pins one value as well as the equality, so
|
||||
a rewrite that is the same wrong answer in every order still fails. Rival 3
|
||||
fails it.
|
||||
|
||||
## `command/cmd/tile.rs` — the tile bar's offered set, by name (improvise-jgzw)
|
||||
|
||||
### the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included
|
||||
@@ -5337,3 +5394,83 @@ Rivals, injected, full workspace run:
|
||||
*else* fails: `renders_each_tile_with_its_axis_label` (TUI) and
|
||||
`the_tile_bar_route_into_records_mode_does_not_strand_the_user`. The second one
|
||||
is improvise-bd9o's own test of the route this decision keeps.
|
||||
|
||||
## `command/cmd/determinism.rs` — the determinism law over every command (improvise-hzjo)
|
||||
|
||||
The reasoning is in `context/code/command.md`, "A command is a function of its
|
||||
context". This entry records what each test can see.
|
||||
|
||||
### every_command_gives_equal_effects_in_independently_built_equal_contexts
|
||||
|
||||
It runs all 89 registrations, 1500+ (command, args) pairs, in four contexts built
|
||||
from four separately parsed copies of `web/demo.improv`, and requires equal
|
||||
effect `Debug` text. It passed at `ae96b8c` with no exemptions. It is not a
|
||||
regression test for a live bug. It restores a law, and the evidence is which
|
||||
rivals it catches:
|
||||
|
||||
- **`command-mode-backspace` reading `ctx.buffers.values().next()`** instead of
|
||||
the `command` buffer: a real command following hash order. Injected, full
|
||||
workspace run: this test is the **only** failure, because every
|
||||
`text_buffer.rs` test builds a one-entry buffer map. With the comparison
|
||||
switched to one reused context (`compare(&a, from_ref(&a))`) the same rival
|
||||
**passes**. That is the half the ticket required: the distinct construction
|
||||
is what does the work.
|
||||
- **`DataStore::matching_positions` without its sort**: gotcha 18 re-shipped.
|
||||
This entry first said the test catches it through `drill-into-cell` and
|
||||
`fit-measure`'s record order. That was wrong (improvise-e1sm). `fit-measure`
|
||||
never diverged, in 60 failing runs in review and in 354 here. And the catch
|
||||
was a matter of luck. The only real drill was `Product/Latte, Quarter/Q2`,
|
||||
which is one record with two cells, so each other context agrees with
|
||||
probability 1/2. With the sort removed and the test binary run in a loop, the
|
||||
law **passed 46 of 400 runs** at `fc0b13c`, and `drill-into-cell
|
||||
["Product/Latte", "Quarter/Q2"]` was the only pair that ever diverged. The
|
||||
arguments now include every item of every regular category as a
|
||||
one-coordinate drill, read off the parsed document (four records, eight cells
|
||||
each in the demo). With the same injection the law **passed 0 of 400**, and
|
||||
all eight new drills diverged in about 1195 of 1200 pairwise comparisons each.
|
||||
Without the injection it passed 400 of 400. The three `cell.rs` order tests
|
||||
still pin the store directly.
|
||||
- **The derived drills dropped from the argument list** (the
|
||||
`all.extend(one_coordinate_drills(model))` line commented out): the test
|
||||
fails before comparing anything, with "the widest drill orders 1 records".
|
||||
That is the only way of losing the wide drills the guard can see. It measures
|
||||
what the argument lists match, not what the built commands ordered, so it can
|
||||
pass while the comparison has lost its wide drills (improvise-b9f2). Review
|
||||
rival: `drill-into-cell`'s parser changed to refuse fewer than two arguments,
|
||||
which sends every one-coordinate drill to the one-record cursor fallback. The
|
||||
guard stays silent and, with gotcha 18 re-injected, the law passed 38 of 300
|
||||
runs.
|
||||
- **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`.
|
||||
It fails as "listed … but did not".
|
||||
|
||||
**A vacuity this test itself had on its first version.** The fixture's filler
|
||||
buffers held `v0`…`v15`. The `command-mode-backspace` rival pops a character,
|
||||
so every value collapsed to `v` (or `v1`), and the rival passed across two
|
||||
distinct contexts. This is the "two derivations the fixture makes agree" shape.
|
||||
The values are now `value {i} of 16`. The comparison was also widened from one
|
||||
other instance to three, because a take-the-first rival agrees across two seeds
|
||||
about one time in 21.
|
||||
|
||||
### only_distinct_instances_can_see_a_command_that_follows_hash_order
|
||||
|
||||
The check-the-checker behind the law. A test-only `EmitsInHashOrder` emits one
|
||||
`SetBuffer` per `ctx.buffers` entry in iteration order. It must equal itself on
|
||||
one context, which is why the old single-context law was vacuous, and differ
|
||||
across two independently built ones. If a future refactor made the fixture
|
||||
share one map, or share one `RandomState`, the second assertion fails. Without
|
||||
this test the law would then pass everything and look healthy.
|
||||
|
||||
## JSON import numbers (improvise-6yvg)
|
||||
|
||||
### `a_json_import_stores_its_numbers_bit_for_bit`
|
||||
|
||||
`ImportJsonHeadless` imports `[{"Region":"East","Revenue":0.24815714285714285}]`
|
||||
and the test reads Region=East, _Measure=Revenue back by `f64::to_bits`.
|
||||
`StartImportWizard` parses through the same `serde_json::from_slice` call, so
|
||||
one test covers the crate's feature.
|
||||
|
||||
**Red at `1158c23`** in `cargo test -p improvise-command`: stored
|
||||
`0.24815714285714283`. That reproduces improvise-6yvg. **Limitation:** this test
|
||||
cannot see the feature removed from `improvise-command` alone, because
|
||||
`improvise-io`, a normal dependency, enables it in the same build. That case is
|
||||
held by `every_crate_that_can_parse_json_enables_float_roundtrip` (`tooling.md`).
|
||||
|
||||
@@ -1361,3 +1361,26 @@ fold is the row `z` names the group from. Note the narrowing this implies: an
|
||||
**empty** collapsed group now has an empty fold row, which `prune_empty` drops, and
|
||||
its header goes with it. lhi4 kept that header because it was the only way back to
|
||||
the rows; the fold is now that way back.
|
||||
|
||||
### records_steps_are_the_spine_then_every_non_index_dim_category_on_none
|
||||
|
||||
improvise-t0ma. Before this, `records_steps` was guarded by one incidental test: the
|
||||
`is_index_or_dim() → is_measure()` rival failed only
|
||||
`save_with_unfinished_record_is_readable`, a persistence test that happens to enter
|
||||
records mode. This asserts the **steps**, not the effects `cmd/grid.rs` translates
|
||||
them into, because the steps are the model-side answer a browser will also consume.
|
||||
The expected list filters `_Index`/`_Dim` by **name**, not by `CategoryKind`, so it
|
||||
is not a second copy of the predicate under test, and the setup asserts `_Measure`
|
||||
and a regular category are both present, since a model with neither could not tell
|
||||
the two predicates apart. Rival: `is_measure()` in place of `is_index_or_dim()`.
|
||||
This test fails, because `_Measure` is dropped and `_Index`/`_Dim` are put on `None`
|
||||
after the spine that just placed them.
|
||||
|
||||
### drill_steps_filter_each_coordinate_and_free_the_rest
|
||||
|
||||
improvise-t0ma's companion for `drill_steps`. The fixture includes a `none_cat` that
|
||||
the key also fixes (`Region`) and one that is virtual (`_Index`), so both exclusions
|
||||
are exercised. It also lists the key's coordinates out of sorted order, which checks
|
||||
that the steps follow `CellKey`'s canonical order. Rival: `Axis::Page` in place of
|
||||
`Axis::Filter` for a fixed coordinate, which reopens improvise-dz8 (a drilled
|
||||
coordinate the user could page away from). This test fails.
|
||||
|
||||
@@ -1434,3 +1434,17 @@ rather than defaulted: the alternative — expanding the group in the export —
|
||||
reintroduce the iot6 divergence between what the user sees and what they export.
|
||||
On the pre-6qet tree the body is empty (the group had no row, and pruning left
|
||||
nothing), so the test fails there.
|
||||
|
||||
## Legacy JSON numbers (improvise-6yvg)
|
||||
|
||||
### `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`
|
||||
|
||||
A workbook with Food/Jan holding `0.24815714285714285` is serialized with
|
||||
`serde_json` and loaded through `load_from`. A leading `{` sends it down the
|
||||
legacy JSON path. The test first asserts that the text holds the exact decimal,
|
||||
so what it measures is the parse. It compares `f64::to_bits`. Found by the
|
||||
review of PR #190, not by the ticket.
|
||||
|
||||
**Red at `1158c23`** in `cargo test -p improvise-io`: loaded
|
||||
`0.24815714285714283`. Green once `improvise-io`'s own manifest enables
|
||||
`float_roundtrip`, and red again with only that line reverted.
|
||||
|
||||
@@ -1159,3 +1159,35 @@ failed: ex_usage_is_placeholders("q or press Q")`. The rule cost one row:
|
||||
`:add-items <cat> a b c ...` had three bare words and is now
|
||||
`:add-items <cat> <item> ...`. This test pins the rule the macro asks, in both
|
||||
directions, including that shipped usage and the rival.
|
||||
|
||||
### help::tests::every_keys_row_drawn_is_one_the_builders_asked_for
|
||||
|
||||
improvise-ssoe, found in re-review of #180 at 544744c. `KeyRow` closed
|
||||
hand-typed keys, but `KeyRow::looked_up` was `pub(super)`, so a page builder
|
||||
could call it directly instead of through its `keys` parameter — and
|
||||
`help_bindings()` records only what goes through the parameter. Injected into
|
||||
`editing()`: `KeyRow::looked_up(n, "clear-cell", None, "Edit the current
|
||||
cell")`, which captions `x` as editing the cell — the exact lie `jhdf`'s test
|
||||
exists for — and the whole workspace stayed green, because both guards that
|
||||
would object read `help_bindings()`.
|
||||
|
||||
Two changes. `looked_up` is now private to the `key_row` module, whose one
|
||||
caller is the module's own `keys` — the lookup `pages()` hands every builder —
|
||||
so the reviewer's injection is `E0624: associated function looked_up is
|
||||
private`. That narrows the door without closing it: a builder can still reach
|
||||
the module-level `keys` by path (`self::keys(..)`) rather than through its
|
||||
shadowing parameter. So this test states the premise directly — the number of
|
||||
keys rows drawn equals the number of rows the builders asked for (every asked
|
||||
row finds its keys, by `every_keys_row_the_pages_ask_for_finds_its_keys`) —
|
||||
and the `self::keys` bypass fails it `51 != 50`.
|
||||
|
||||
**improvise-aszt: a count is not the premise.** The test above compared
|
||||
*counts*, so a bypass that also drops one real row keeps them equal. Injected in
|
||||
`editing()`: call the parameter `keys(e, "clear-buffer", …)` and discard it with
|
||||
`let _ =`, then return `self::keys(n, "clear-cell", None, "Edit the current
|
||||
cell")` in its place. The asked row is recorded and never drawn, the drawn row
|
||||
is never recorded, 50 = 50, and the whole workspace stayed green. The test now
|
||||
compares **content in order**: the drawn keys rows must equal
|
||||
`help_bindings()` looked up through the same `keys`, row for row. The same
|
||||
injection now fails. What still passes is a bypass that draws exactly what the
|
||||
builders asked for, in the order they asked — which is harmless by definition.
|
||||
|
||||
@@ -681,3 +681,41 @@ reproduction.
|
||||
alone fails, exiting 0. All 44 others pass. That is the review's point: the
|
||||
older `a head that moves mid-wait is not a pass` starts from MERGEABLE, so it
|
||||
cannot see this rival.
|
||||
|
||||
## Every JSON parser reads numbers exactly (improvise-dz8c)
|
||||
|
||||
### `every_crate_that_can_parse_json_enables_float_roundtrip`
|
||||
|
||||
It replaces a check over three crates listed by hand. It reads
|
||||
`cargo metadata --no-deps` and fails naming every workspace package, and
|
||||
every target-specific entry, whose **normal** serde_json dependency lacks
|
||||
`float_roundtrip`. The rule and why it was chosen over the alternatives are in
|
||||
`context/code/protocol.md`. It is red at `1158c23`, naming `improvise`,
|
||||
`improvise-command`, `improvise-io` and `improvise-protocol`.
|
||||
|
||||
**Faults injected, each run:**
|
||||
|
||||
- **The feature removed from `improvise-io` alone.** The check fails naming
|
||||
`improvise-io (all targets)`, and so does
|
||||
`a_legacy_json_workbook_loads_its_numbers_bit_for_bit`.
|
||||
`a_json_import_stores_its_numbers_bit_for_bit` stays green, because
|
||||
`improvise-command` enables the feature and unification reaches io in its
|
||||
build.
|
||||
- **The feature removed from `improvise-command` alone.** The check fails naming
|
||||
`improvise-command (all targets)`. The command crate's own import test
|
||||
**stays green**: `cargo tree -p improvise-command` shows io's declaration
|
||||
switching the feature on. So the manifest check is the only guard that
|
||||
sees this case. It is also the case the retired build-graph check would
|
||||
have passed.
|
||||
- **A throwaway workspace member**, `zz-throwaway`, with normal dependencies on
|
||||
`improvise-protocol` and `serde_json` and no feature (the lockfile updated,
|
||||
as a real new crate would be). The check fails naming
|
||||
`zz-throwaway (all targets)`. With the unconditional dependency carrying the
|
||||
feature and a `cfg(target_arch = "wasm32")` one lacking it, the check fails
|
||||
naming `zz-throwaway (cfg(target_arch = "wasm32"))`. That is stricter than
|
||||
the build needs, since a wasm build unifies the two entries, and cheap to
|
||||
satisfy. The crate was then removed.
|
||||
|
||||
The `checked.len() >= 2` assertion is the positive signal. If a cargo change
|
||||
renamed the metadata fields, the check would find no serde_json dependency and
|
||||
pass vacuously without it.
|
||||
|
||||
@@ -14,7 +14,9 @@ improvise-io = { path = "../improvise-io" }
|
||||
improvise-gp = { path = "../improvise-gp" }
|
||||
anyhow = "1"
|
||||
serde = { version = "1", features = ["derive"] }
|
||||
serde_json = "1"
|
||||
# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate,
|
||||
# because each build stands alone (improvise-6yvg, context/code/protocol.md).
|
||||
serde_json = { version = "1", features = ["float_roundtrip"] }
|
||||
unicode-width = "^0.2"
|
||||
# Instant::now() panics on wasm32-unknown-unknown; web_time re-exports
|
||||
# std::time off-wasm and uses performance.now() on it (improvise-ywd).
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
use crate::model::category::MEASURE_CATEGORY;
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::model::cell::CellKey;
|
||||
use crate::ui::app::AppMode;
|
||||
use crate::ui::effect::{self, Effect};
|
||||
use crate::view::{RecordCol, RecordRow};
|
||||
use crate::view::RecordCol;
|
||||
|
||||
use super::core::{Cmd, CmdContext, read_buffer};
|
||||
use super::grid::AddRecordRow;
|
||||
@@ -14,6 +14,7 @@ mod tests {
|
||||
|
||||
use super::*;
|
||||
use crate::command::cmd::test_helpers::*;
|
||||
use crate::model::cell::CellValue;
|
||||
use crate::workbook::Workbook;
|
||||
|
||||
fn commit(kind: CommitKind) -> CommitBuffer {
|
||||
@@ -622,47 +623,8 @@ fn stage_drill_edit(record_idx: usize, col: RecordCol, value: &str) -> Box<dyn E
|
||||
})
|
||||
}
|
||||
|
||||
/// Rename one coordinate of a records row. In wide format a row is a group of cells,
|
||||
/// so the rename moves **every** one of them (improvise-c5v, gotcha 1).
|
||||
fn rename_record_coordinate(
|
||||
model: &crate::model::Model,
|
||||
row: &RecordRow,
|
||||
category: &str,
|
||||
value: &str,
|
||||
effects: &mut Vec<Box<dyn Effect>>,
|
||||
) {
|
||||
if value.is_empty() {
|
||||
effects.push(effect::refuse(effect::RECORD_COORDS_CANNOT_BE_EMPTY));
|
||||
return;
|
||||
}
|
||||
effects.push(Box::new(effect::AddItem {
|
||||
category: category.to_string(),
|
||||
item: value.to_string(),
|
||||
}));
|
||||
|
||||
// Every cell in the row moves. A row seeded by `add-record-row` carries its
|
||||
// coordinates on a lone measure-less cell, which must move too.
|
||||
let cells: Vec<(CellKey, CellValue)> = if row.values.is_empty() {
|
||||
model
|
||||
.get_cell(&row.coords)
|
||||
.map(|v| vec![(row.coords.clone(), v.clone())])
|
||||
.unwrap_or_default()
|
||||
} else {
|
||||
row.values
|
||||
.iter()
|
||||
.map(|(measure, v)| (row.cell_key(measure), v.clone()))
|
||||
.collect()
|
||||
};
|
||||
for (old_key, cell_value) in cells {
|
||||
effects.push(Box::new(effect::ClearCell(old_key.clone())));
|
||||
effects.push(Box::new(effect::SetCell(
|
||||
old_key.with(category, value),
|
||||
cell_value,
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
/// Apply a records-mode edit directly to the model (no drill state).
|
||||
/// Apply a records-mode edit directly to the model (no drill state). A drill's staged
|
||||
/// edits go through the same `RecordRewrite`, all of a row's at once (gotcha 19).
|
||||
fn commit_plain_records_edit(
|
||||
ctx: &CmdContext,
|
||||
record_idx: usize,
|
||||
@@ -674,11 +636,8 @@ fn commit_plain_records_edit(
|
||||
return;
|
||||
};
|
||||
match col {
|
||||
RecordCol::Category(category) => {
|
||||
rename_record_coordinate(ctx.model, row, category, value, effects);
|
||||
}
|
||||
RecordCol::Measure(measure) => {
|
||||
commit_regular_cell_value(&row.cell_key(measure), value, effects);
|
||||
RecordCol::Category(_) | RecordCol::Measure(_) => {
|
||||
effects.extend(effect::RecordRewrite::of(ctx.model, row, [(col, value)]).effects());
|
||||
}
|
||||
RecordCol::NewMeasure => add_measure(value, effects),
|
||||
RecordCol::NewCategory => add_category(value, effects),
|
||||
|
||||
@@ -0,0 +1,233 @@
|
||||
//! A command is a function of its context: two contexts built independently from
|
||||
//! equal data give equal effects — `context/code/command.md`, improvise-hzjo.
|
||||
|
||||
use std::collections::{BTreeSet, HashSet};
|
||||
|
||||
use super::core::{Cmd, CmdContext};
|
||||
use super::registry::tests::candidate_args;
|
||||
use crate::command::key::{KeyCode, KeyModifiers};
|
||||
use crate::model::Model;
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::model::record::record_of;
|
||||
use crate::ui::app::App;
|
||||
use crate::ui::effect::{self, Effect};
|
||||
|
||||
/// Commands whose effects depend on something outside the context, with why.
|
||||
const OUTSIDE_THE_CONTEXT: &[(&str, &str)] = &[];
|
||||
|
||||
const DOCUMENT: &str = include_str!("../../../../../web/demo.improv");
|
||||
|
||||
/// One `App` over the shipped demo. Every context map holds several entries that
|
||||
/// stay distinct under an edit like `pop()` — see the evidence.
|
||||
fn fixture() -> App {
|
||||
let workbook = crate::persistence::parse_md(DOCUMENT).expect("the demo parses");
|
||||
let mut app = App::new(workbook, None);
|
||||
let vs = &mut app.view_state;
|
||||
for i in 0..16 {
|
||||
vs.buffers
|
||||
.insert(format!("b{i}"), format!("value {i} of 16"));
|
||||
}
|
||||
for (name, text) in [
|
||||
("edit", "42"),
|
||||
("formula", "Margin = Profit / Revenue"),
|
||||
("category", "Region"),
|
||||
("command", "add-item Product Mocha"),
|
||||
("export", "/tmp/improvise-hzjo.csv"),
|
||||
] {
|
||||
vs.buffers.insert(name.into(), text.into());
|
||||
}
|
||||
for cat in ["Product", "Quarter", "_Measure", "_Index", "_Dim"] {
|
||||
vs.expanded_cats.insert(cat.into());
|
||||
}
|
||||
vs.yanked = Some(CellValue::Number(7.0));
|
||||
vs.formula_panel_open = true;
|
||||
vs.category_panel_open = true;
|
||||
vs.view_panel_open = true;
|
||||
vs.cat_panel_cursor = 2;
|
||||
vs.view_panel_cursor = 1;
|
||||
app.cursor_mut().selected = (1, 1);
|
||||
app
|
||||
}
|
||||
|
||||
/// Every item as a one-coordinate drill: each spans the other categories whole,
|
||||
/// so the store has several records to order — see the evidence.
|
||||
fn one_coordinate_drills(model: &Model) -> Vec<Vec<String>> {
|
||||
let mut drills = Vec::new();
|
||||
for cat in model.regular_category_names() {
|
||||
for item in model.category(cat).expect("listed").ordered_item_names() {
|
||||
drills.push(vec![format!("{cat}/{item}")]);
|
||||
}
|
||||
}
|
||||
drills
|
||||
}
|
||||
|
||||
/// How many distinct records the widest all-`Cat/Item` candidate matches.
|
||||
fn widest_drill(model: &Model, candidates: &[Vec<String>]) -> usize {
|
||||
let coordinates = |args: &Vec<String>| -> Option<Vec<(String, String)>> {
|
||||
args.iter()
|
||||
.map(|a| {
|
||||
a.split_once('/')
|
||||
.map(|(c, i)| (c.to_string(), i.to_string()))
|
||||
})
|
||||
.collect()
|
||||
};
|
||||
candidates
|
||||
.iter()
|
||||
.filter(|args| !args.is_empty())
|
||||
.filter_map(coordinates)
|
||||
.map(|partial| {
|
||||
let cells = model.data.matching_cells(&partial);
|
||||
let records: HashSet<CellKey> = cells.iter().map(|(k, _)| record_of(k)).collect();
|
||||
records.len()
|
||||
})
|
||||
.max()
|
||||
.unwrap_or(0)
|
||||
}
|
||||
|
||||
/// The registry's candidates, plus names the demo really has: a drill or a fit
|
||||
/// over coordinates that match nothing never reaches the records it would order.
|
||||
fn args_the_document_names(model: &Model) -> Vec<Vec<String>> {
|
||||
let mut all = candidate_args();
|
||||
all.extend(one_coordinate_drills(model));
|
||||
all.extend(
|
||||
[
|
||||
vec!["Revenue"],
|
||||
vec!["Product/Latte", "Quarter/Q2"],
|
||||
vec!["Product/Latte", "Quarter/Q2", "_Measure/Cost"],
|
||||
vec![
|
||||
"editing",
|
||||
"5",
|
||||
"Product/Latte",
|
||||
"Quarter/Q2",
|
||||
"_Measure/Cost",
|
||||
],
|
||||
]
|
||||
.into_iter()
|
||||
.map(|v| v.into_iter().map(String::from).collect()),
|
||||
);
|
||||
all
|
||||
}
|
||||
|
||||
fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option<String> {
|
||||
let cmd = ctx.registry.interactive(name, args, ctx).ok()?;
|
||||
Some(format!("{:?}", cmd.execute(ctx)))
|
||||
}
|
||||
|
||||
/// What running every registered command under `a` and under each of `others` found.
|
||||
#[derive(Default)]
|
||||
struct Comparison {
|
||||
diverged: BTreeSet<&'static str>,
|
||||
shown: Vec<String>,
|
||||
never_built: Vec<&'static str>,
|
||||
compared: usize,
|
||||
}
|
||||
|
||||
fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison {
|
||||
let mut found = Comparison::default();
|
||||
let candidates = args_the_document_names(a.model);
|
||||
for name in a.registry.names().collect::<Vec<_>>() {
|
||||
let before = found.compared;
|
||||
for args in &candidates {
|
||||
let Some(left) = effects_of(name, args, a) else {
|
||||
continue;
|
||||
};
|
||||
found.compared += 1;
|
||||
for b in others {
|
||||
let right = effects_of(name, args, b).unwrap_or_default();
|
||||
if left != right {
|
||||
found.diverged.insert(name);
|
||||
found
|
||||
.shown
|
||||
.push(format!("{name} {args:?}:\n {left}\n {right}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
if found.compared == before {
|
||||
found.never_built.push(name);
|
||||
}
|
||||
}
|
||||
found
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/command.md`.
|
||||
#[test]
|
||||
fn every_command_gives_equal_effects_in_independently_built_equal_contexts() {
|
||||
let apps: Vec<App> = (0..4).map(|_| fixture()).collect();
|
||||
let contexts: Vec<CmdContext> = apps
|
||||
.iter()
|
||||
.map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE))
|
||||
.collect();
|
||||
let model = contexts[0].model;
|
||||
let widest = widest_drill(model, &args_the_document_names(model));
|
||||
assert!(
|
||||
widest >= 4,
|
||||
"the widest drill orders {widest} records, so the store's order is nearly a coin toss"
|
||||
);
|
||||
let found = compare(&contexts[0], &contexts[1..]);
|
||||
assert!(
|
||||
found.never_built.is_empty(),
|
||||
"no candidate args build these in the fixture, so the law never ran them: {:?}",
|
||||
found.never_built
|
||||
);
|
||||
let listed: BTreeSet<&str> = OUTSIDE_THE_CONTEXT.iter().map(|(n, _)| *n).collect();
|
||||
let unlisted: Vec<&str> = found
|
||||
.shown
|
||||
.iter()
|
||||
.filter(|s| !listed.iter().any(|n| s.starts_with(&format!("{n} "))))
|
||||
.map(String::as_str)
|
||||
.collect();
|
||||
assert!(
|
||||
unlisted.is_empty(),
|
||||
"effects depend on more than the context ({} pairs compared):\n{}",
|
||||
found.compared,
|
||||
unlisted.join("\n")
|
||||
);
|
||||
let stale: Vec<&&str> = listed
|
||||
.iter()
|
||||
.filter(|n| !found.diverged.contains(**n))
|
||||
.collect();
|
||||
assert!(
|
||||
stale.is_empty(),
|
||||
"listed as depending on something outside the context, but did not: {stale:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Iterates a context `HashMap` and emits in that order: the one nondeterminism
|
||||
/// this repo has shipped (gotcha 18). The law must see it; a reused context cannot.
|
||||
#[derive(Debug)]
|
||||
struct EmitsInHashOrder;
|
||||
impl Cmd for EmitsInHashOrder {
|
||||
fn name(&self) -> &'static str {
|
||||
"emits-in-hash-order"
|
||||
}
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
ctx.buffers
|
||||
.iter()
|
||||
.map(|(name, value)| {
|
||||
Box::new(effect::SetBuffer {
|
||||
name: name.clone(),
|
||||
value: value.clone(),
|
||||
}) as Box<dyn Effect>
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/command.md`.
|
||||
#[test]
|
||||
fn only_distinct_instances_can_see_a_command_that_follows_hash_order() {
|
||||
let (first, second) = (fixture(), fixture());
|
||||
let a = first.cmd_context(KeyCode::Null, KeyModifiers::NONE);
|
||||
let b = second.cmd_context(KeyCode::Null, KeyModifiers::NONE);
|
||||
let run = |ctx: &CmdContext| format!("{:?}", EmitsInHashOrder.execute(ctx));
|
||||
assert_eq!(
|
||||
run(&a),
|
||||
run(&a),
|
||||
"one reused context iterates its maps the same way twice"
|
||||
);
|
||||
assert_ne!(
|
||||
run(&a),
|
||||
run(&b),
|
||||
"two independently built contexts must iterate differently, or the law is vacuous"
|
||||
);
|
||||
}
|
||||
@@ -13,6 +13,9 @@ pub mod text_buffer;
|
||||
pub mod tile;
|
||||
pub mod wizard;
|
||||
|
||||
#[cfg(test)]
|
||||
mod determinism;
|
||||
|
||||
// Re-export items used by external code
|
||||
pub use self::core::{Cmd, CmdContext, CmdRegistry};
|
||||
pub use registry::default_registry;
|
||||
|
||||
@@ -498,13 +498,13 @@ pub fn default_registry() -> CmdRegistry {
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
pub(super) mod tests {
|
||||
use super::*;
|
||||
use crate::command::cmd::test_helpers::*;
|
||||
|
||||
/// Argument vectors to try when constructing a command, most specific
|
||||
/// first. A command is exercised by the first one its parser accepts.
|
||||
fn candidate_args() -> Vec<Vec<String>> {
|
||||
pub(in crate::command::cmd) fn candidate_args() -> Vec<Vec<String>> {
|
||||
[
|
||||
vec![],
|
||||
vec!["1"],
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use std::collections::BTreeMap;
|
||||
use std::fmt::Debug;
|
||||
use std::path::PathBuf;
|
||||
|
||||
@@ -663,24 +664,19 @@ impl Effect for ApplyAndClearDrill {
|
||||
let Some(drill) = app.drill_state.take() else {
|
||||
return;
|
||||
};
|
||||
if drill.pending_edits.is_empty() {
|
||||
return;
|
||||
}
|
||||
let mut rows: BTreeMap<usize, Vec<(&RecordCol, &str)>> = BTreeMap::new();
|
||||
for ((record_idx, col), new_value) in &drill.pending_edits {
|
||||
let Some(row) = drill.records.get(*record_idx) else {
|
||||
continue;
|
||||
};
|
||||
match col {
|
||||
RecordCol::Measure(measure) => {
|
||||
apply_measure_edit(app, &row.cell_key(measure), new_value);
|
||||
}
|
||||
RecordCol::Category(category) => {
|
||||
apply_coordinate_rename(app, row, category, new_value);
|
||||
}
|
||||
// Never staged: `commit_cell_value` applies a new measure or
|
||||
// category immediately — a schema change is not a cell edit.
|
||||
RecordCol::NewMeasure | RecordCol::NewCategory => {}
|
||||
}
|
||||
rows.entry(*record_idx)
|
||||
.or_default()
|
||||
.push((col, new_value.as_str()));
|
||||
}
|
||||
let model = &app.model_state.workbook.model;
|
||||
let rewrite = RecordRewrite::merged(rows.iter().filter_map(|(record_idx, edits)| {
|
||||
let row = drill.records.get(*record_idx)?;
|
||||
Some(RecordRewrite::of(model, row, edits.iter().copied()))
|
||||
}));
|
||||
for effect in rewrite.effects() {
|
||||
effect.apply(app);
|
||||
}
|
||||
// No `dirty = true` here: this declares `Model(CellData)` and
|
||||
// `App::apply_effects` raises the flag off that (improvise-gv9r).
|
||||
@@ -690,51 +686,114 @@ impl Effect for ApplyAndClearDrill {
|
||||
}
|
||||
}
|
||||
|
||||
/// Write one measure cell, or clear it when the edit is blank.
|
||||
fn apply_measure_edit(app: &mut App, key: &CellKey, new_value: &str) {
|
||||
let model = &mut app.model_state.workbook.model;
|
||||
match new_value {
|
||||
"" => {
|
||||
model.clear_cell(key);
|
||||
}
|
||||
v => {
|
||||
let value = v
|
||||
.parse::<f64>()
|
||||
.map(CellValue::Number)
|
||||
.unwrap_or_else(|_| CellValue::Text(v.to_string()));
|
||||
model.set_cell(key.clone(), value);
|
||||
}
|
||||
}
|
||||
/// A records row's edits composed into the one record they mean, read off the model
|
||||
/// before anything is written. Why composed: `context/code/command.md`, `RecordRewrite`.
|
||||
#[derive(Debug, Default, PartialEq)]
|
||||
pub(crate) struct RecordRewrite {
|
||||
refused: bool,
|
||||
items: Vec<(String, String)>,
|
||||
clears: Vec<CellKey>,
|
||||
sets: Vec<(CellKey, CellValue)>,
|
||||
}
|
||||
|
||||
/// Rename one coordinate of a records row. The row is a group of cells —
|
||||
/// one per measure — so every one of them moves (improvise-c5v).
|
||||
fn apply_coordinate_rename(app: &mut App, row: &RecordRow, category: &str, new_value: &str) {
|
||||
if new_value.is_empty() {
|
||||
app.refuse(RECORD_COORDS_CANNOT_BE_EMPTY);
|
||||
return;
|
||||
}
|
||||
let keys: Vec<CellKey> = if row.values.is_empty() {
|
||||
vec![row.coords.clone()]
|
||||
} else {
|
||||
row.values
|
||||
.iter()
|
||||
.map(|(measure, _)| row.cell_key(measure))
|
||||
.collect()
|
||||
};
|
||||
let _ = app
|
||||
.model_state
|
||||
.workbook
|
||||
.add_item(category, new_value.to_string());
|
||||
for old_key in keys {
|
||||
let Some(value) = app.model_state.workbook.model.get_cell(&old_key).cloned() else {
|
||||
continue;
|
||||
impl RecordRewrite {
|
||||
/// `row`'s edits, each naming a distinct column. The result does not depend on
|
||||
/// their order: every coordinate is renamed before any cell is placed.
|
||||
pub(crate) fn of<'a>(
|
||||
model: &crate::model::Model,
|
||||
row: &RecordRow,
|
||||
edits: impl IntoIterator<Item = (&'a RecordCol, &'a str)>,
|
||||
) -> Self {
|
||||
let mut refused = false;
|
||||
let mut renames: BTreeMap<&str, &str> = BTreeMap::new();
|
||||
let mut measures: BTreeMap<&str, Option<CellValue>> = BTreeMap::new();
|
||||
for (col, value) in edits {
|
||||
match col {
|
||||
RecordCol::Category(_) if value.is_empty() => refused = true,
|
||||
RecordCol::Category(category) => {
|
||||
renames.insert(category.as_str(), value);
|
||||
}
|
||||
RecordCol::Measure(measure) => {
|
||||
measures.insert(measure.as_str(), improvise_protocol::typed(value));
|
||||
}
|
||||
// Never staged: a new measure or category is a schema change,
|
||||
// applied at once by `commit_cell_value`.
|
||||
RecordCol::NewMeasure | RecordCol::NewCategory => {}
|
||||
}
|
||||
}
|
||||
let renamed = |key: &CellKey| {
|
||||
renames.iter().fold(key.clone(), |key, (category, item)| {
|
||||
key.with(*category, *item)
|
||||
})
|
||||
};
|
||||
app.model_state.workbook.model.clear_cell(&old_key);
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.set_cell(old_key.with(category, new_value), value);
|
||||
let moved: Vec<(Option<&str>, CellKey)> = match (renames.is_empty(), row.values.is_empty())
|
||||
{
|
||||
(true, _) => Vec::new(),
|
||||
(false, true) => vec![(None, row.coords.clone())],
|
||||
(false, false) => row
|
||||
.values
|
||||
.iter()
|
||||
.map(|(measure, _)| (Some(measure.as_str()), row.cell_key(measure)))
|
||||
.collect(),
|
||||
};
|
||||
let mut rewrite = RecordRewrite {
|
||||
refused,
|
||||
items: renames
|
||||
.iter()
|
||||
.map(|(category, item)| (category.to_string(), item.to_string()))
|
||||
.collect(),
|
||||
..Self::default()
|
||||
};
|
||||
for (measure, old) in &moved {
|
||||
let value = match measure.and_then(|m| measures.remove(m)) {
|
||||
Some(edit) => edit,
|
||||
None => model.get_cell(old).cloned(),
|
||||
};
|
||||
rewrite.clears.push(old.clone());
|
||||
rewrite.sets.extend(value.map(|v| (renamed(old), v)));
|
||||
}
|
||||
for (measure, edit) in measures {
|
||||
let key = renamed(&row.cell_key(measure));
|
||||
match edit {
|
||||
Some(value) => rewrite.sets.push((key, value)),
|
||||
None => rewrite.clears.push(key),
|
||||
}
|
||||
}
|
||||
rewrite
|
||||
}
|
||||
|
||||
/// Several rows' rewrites as one: every clear lands before any write, so a
|
||||
/// swap is a swap, and where two rows land on one cell the later row wins.
|
||||
pub(crate) fn merged(rewrites: impl IntoIterator<Item = Self>) -> Self {
|
||||
rewrites.into_iter().fold(Self::default(), |mut all, one| {
|
||||
all.refused |= one.refused;
|
||||
all.items.extend(one.items);
|
||||
all.clears.extend(one.clears);
|
||||
all.sets.extend(one.sets);
|
||||
all
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn effects(self) -> Vec<Box<dyn Effect>> {
|
||||
let refusal = self.refused.then(|| refuse(RECORD_COORDS_CANNOT_BE_EMPTY));
|
||||
let items = self
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|(category, item)| Box::new(AddItem { category, item }) as Box<dyn Effect>);
|
||||
let clears = self
|
||||
.clears
|
||||
.into_iter()
|
||||
.map(|key| Box::new(ClearCell(key)) as Box<dyn Effect>);
|
||||
let sets = self
|
||||
.sets
|
||||
.into_iter()
|
||||
.map(|(key, value)| Box::new(SetCell(key, value)) as Box<dyn Effect>);
|
||||
refusal
|
||||
.into_iter()
|
||||
.chain(items)
|
||||
.chain(clears)
|
||||
.chain(sets)
|
||||
.collect()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3071,6 +3130,209 @@ mod tests {
|
||||
assert_eq!(app.model_state.workbook.model.get_cell(&key), None);
|
||||
}
|
||||
|
||||
// ── Composing a drill's staged edits (improvise-uo2p) ───────────────
|
||||
|
||||
fn drill_cell(ty: &str, month: &str, measure: &str) -> CellKey {
|
||||
CellKey::new(vec![
|
||||
("Type".into(), ty.into()),
|
||||
("Month".into(), month.into()),
|
||||
("_Measure".into(), measure.into()),
|
||||
])
|
||||
}
|
||||
|
||||
/// Two records, each carrying Revenue and Cost: row 0 is Food/Jan (1, 2),
|
||||
/// row 1 is Clothing/Jan (42, 17). Every edit below is staged on row 1 at least.
|
||||
fn two_record_drill() -> App {
|
||||
let mut app = test_app();
|
||||
let cells = [
|
||||
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
|
||||
(drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)),
|
||||
(
|
||||
drill_cell("Clothing", "Jan", "Revenue"),
|
||||
CellValue::Number(42.0),
|
||||
),
|
||||
(
|
||||
drill_cell("Clothing", "Jan", "Cost"),
|
||||
CellValue::Number(17.0),
|
||||
),
|
||||
];
|
||||
for (key, value) in cells.clone() {
|
||||
app.model_state.workbook.model.set_cell(key, value);
|
||||
}
|
||||
let records = RecordRow::group(cells);
|
||||
assert_eq!(records.len(), 2, "one row per coordinate tuple");
|
||||
assert_eq!(records[1].coords.get("Type"), Some("Clothing"));
|
||||
StartDrill(records).apply(&mut app);
|
||||
app
|
||||
}
|
||||
|
||||
fn stage(app: &mut App, record_idx: usize, col: RecordCol, value: &str) {
|
||||
SetDrillPendingEdit {
|
||||
record_idx,
|
||||
col,
|
||||
new_value: value.to_string(),
|
||||
}
|
||||
.apply(app);
|
||||
}
|
||||
|
||||
/// The whole store, as `(Type, Month, measure) = n`, so a ghost cannot hide.
|
||||
fn drill_store(app: &App) -> std::collections::BTreeSet<String> {
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.data
|
||||
.iter_cells()
|
||||
.map(|(key, value)| {
|
||||
let coord = |c: &str| key.get(c).unwrap_or("-").to_string();
|
||||
let n = value.as_f64().expect("every drill cell here is a number");
|
||||
format!(
|
||||
"{}/{}/{} = {n}",
|
||||
coord("Type"),
|
||||
coord("Month"),
|
||||
coord("_Measure")
|
||||
)
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn category(name: &str) -> RecordCol {
|
||||
RecordCol::Category(name.into())
|
||||
}
|
||||
|
||||
fn measure(name: &str) -> RecordCol {
|
||||
RecordCol::Measure(name.into())
|
||||
}
|
||||
|
||||
/// Stage `edits` and leave the drill, in 32 fresh Apps: each `HashMap` gets its
|
||||
/// own `RandomState`, and one map iterates the same way twice.
|
||||
fn assert_drill_commits_to(edits: &[(usize, RecordCol, &str)], expected: &[&str]) {
|
||||
let expected: std::collections::BTreeSet<String> =
|
||||
expected.iter().map(|s| s.to_string()).collect();
|
||||
for attempt in 0..32 {
|
||||
let mut app = two_record_drill();
|
||||
for (row, col, value) in edits {
|
||||
stage(&mut app, *row, col.clone(), value);
|
||||
}
|
||||
app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
|
||||
assert_eq!(drill_store(&app), expected, "fresh App #{attempt}");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_drill_row_renamed_twice_and_edited_lands_as_one_record() {
|
||||
assert_drill_commits_to(
|
||||
&[
|
||||
(1, measure("Cost"), "999"),
|
||||
(1, category("Type"), "Drink"),
|
||||
(1, category("Month"), "Feb"),
|
||||
],
|
||||
&[
|
||||
"Food/Jan/Revenue = 1",
|
||||
"Food/Jan/Cost = 2",
|
||||
"Drink/Feb/Revenue = 42",
|
||||
"Drink/Feb/Cost = 999",
|
||||
],
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_drill_rows_measure_edits_follow_its_rename() {
|
||||
assert_drill_commits_to(
|
||||
&[
|
||||
(1, measure("Revenue"), "100"),
|
||||
(1, measure("Cost"), "999"),
|
||||
(1, category("Type"), "Drink"),
|
||||
],
|
||||
&[
|
||||
"Food/Jan/Revenue = 1",
|
||||
"Food/Jan/Cost = 2",
|
||||
"Drink/Jan/Revenue = 100",
|
||||
"Drink/Jan/Cost = 999",
|
||||
],
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_drill_that_swaps_two_records_coordinates_swaps_them() {
|
||||
assert_drill_commits_to(
|
||||
&[
|
||||
(0, category("Type"), "Clothing"),
|
||||
(1, category("Type"), "Food"),
|
||||
],
|
||||
&[
|
||||
"Clothing/Jan/Revenue = 1",
|
||||
"Clothing/Jan/Cost = 2",
|
||||
"Food/Jan/Revenue = 42",
|
||||
"Food/Jan/Cost = 17",
|
||||
],
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates() {
|
||||
assert_drill_commits_to(
|
||||
&[
|
||||
(0, category("Type"), "Drink"),
|
||||
(1, category("Type"), "Food"),
|
||||
(1, measure("Cost"), "999"),
|
||||
],
|
||||
&[
|
||||
"Drink/Jan/Revenue = 1",
|
||||
"Drink/Jan/Cost = 2",
|
||||
"Food/Jan/Revenue = 42",
|
||||
"Food/Jan/Cost = 999",
|
||||
],
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_record_rewrite_is_the_same_under_every_order_of_its_edits() {
|
||||
let app = two_record_drill();
|
||||
let row = app.drill_state.as_ref().unwrap().records[1].clone();
|
||||
let edits = [
|
||||
(measure("Cost"), "999"),
|
||||
(category("Type"), "Drink"),
|
||||
(measure("Revenue"), ""),
|
||||
(category("Month"), "Feb"),
|
||||
];
|
||||
let model = &app.model_state.workbook.model;
|
||||
let mut seen = Vec::new();
|
||||
for a in 0..4 {
|
||||
for b in (0..4).filter(|&b| b != a) {
|
||||
for c in (0..4).filter(|&c| c != a && c != b) {
|
||||
let d = 6 - a - b - c;
|
||||
let order = [a, b, c, d].map(|i| (&edits[i].0, edits[i].1));
|
||||
seen.push(RecordRewrite::of(model, &row, order));
|
||||
}
|
||||
}
|
||||
}
|
||||
assert_eq!(seen.len(), 24);
|
||||
let first = &seen[0];
|
||||
assert_eq!(
|
||||
first.clears,
|
||||
vec![
|
||||
drill_cell("Clothing", "Jan", "Revenue"),
|
||||
drill_cell("Clothing", "Jan", "Cost")
|
||||
]
|
||||
);
|
||||
assert_eq!(
|
||||
first.sets,
|
||||
vec![(drill_cell("Drink", "Feb", "Cost"), CellValue::Number(999.0))]
|
||||
);
|
||||
assert!(seen.iter().all(|rewrite| rewrite == first));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row() {
|
||||
assert_drill_commits_to(
|
||||
&[
|
||||
(0, category("Type"), "Drink"),
|
||||
(1, category("Type"), "Drink"),
|
||||
],
|
||||
&["Drink/Jan/Revenue = 42", "Drink/Jan/Cost = 17"],
|
||||
);
|
||||
}
|
||||
|
||||
// ── Toggle effects ──────────────────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
@@ -3283,6 +3545,38 @@ mod tests {
|
||||
assert_eq!(app.view_state.status_msg, "Imported successfully");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_json_import_stores_its_numbers_bit_for_bit() {
|
||||
const LOSSY: f64 = 0.248_157_142_857_142_85;
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let path = dir.path().join("data.json");
|
||||
std::fs::write(
|
||||
&path,
|
||||
r#"[{"Region":"East","Revenue":0.24815714285714285}]"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let mut app = test_app();
|
||||
ImportJsonHeadless {
|
||||
path,
|
||||
model_name: None,
|
||||
array_path: None,
|
||||
}
|
||||
.apply(&mut app);
|
||||
|
||||
let key = CellKey::new(vec![
|
||||
("Region".to_string(), "East".to_string()),
|
||||
("_Measure".to_string(), "Revenue".to_string()),
|
||||
]);
|
||||
let Some(CellValue::Number(n)) = app.model_state.workbook.model.get_cell(&key) else {
|
||||
panic!(
|
||||
"the import must store the number; status {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
};
|
||||
assert_eq!(n.to_bits(), LOSSY.to_bits(), "stored {n:?}, not {LOSSY:?}");
|
||||
}
|
||||
|
||||
// ── json_import_records (improvise-oaq helper) ──────────────────────
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -1741,4 +1741,66 @@ mod tests {
|
||||
"Sep is in no group — Q2's GroupEnd closed Q1's re-opened run"
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-t0ma: the steps themselves, not their translation into effects —
|
||||
/// see context/test-evidence/core.md.
|
||||
#[test]
|
||||
fn records_steps_are_the_spine_then_every_non_index_dim_category_on_none() {
|
||||
use super::{ViewStep, records_spine, records_steps};
|
||||
let mut m = crate::model::Model::new("T");
|
||||
m.add_category("Region").unwrap();
|
||||
m.add_category("Product").unwrap();
|
||||
let none = |c: &str| ViewStep::Axis {
|
||||
category: c.to_string(),
|
||||
axis: Axis::None,
|
||||
};
|
||||
let mut expected = records_spine();
|
||||
expected.extend(
|
||||
m.categories
|
||||
.keys()
|
||||
.filter(|c| !matches!(c.as_str(), "_Index" | "_Dim"))
|
||||
.map(|c| none(c)),
|
||||
);
|
||||
assert!(
|
||||
expected.contains(&none("_Measure")) && expected.contains(&none("Region")),
|
||||
"setup: the model carries _Measure and a regular category"
|
||||
);
|
||||
assert_eq!(records_steps(&m), expected);
|
||||
}
|
||||
|
||||
/// improvise-t0ma: a drill FIXES its coordinates on Filter and frees only the
|
||||
/// unfixed, non-virtual aggregated categories.
|
||||
#[test]
|
||||
fn drill_steps_filter_each_coordinate_and_free_the_rest() {
|
||||
use super::{ViewStep, drill_steps, records_spine};
|
||||
let key = CellKey::new(vec![
|
||||
("Region".into(), "North".into()),
|
||||
("Month".into(), "Feb".into()),
|
||||
("_Measure".into(), "Revenue".into()),
|
||||
]);
|
||||
let none_cats: Vec<String> = ["Product", "Region", "_Index", "Customer"]
|
||||
.map(String::from)
|
||||
.to_vec();
|
||||
let fixed = |c: &str, i: &str| {
|
||||
[
|
||||
ViewStep::Axis {
|
||||
category: c.into(),
|
||||
axis: Axis::Filter,
|
||||
},
|
||||
ViewStep::PageSelection {
|
||||
category: c.into(),
|
||||
item: i.into(),
|
||||
},
|
||||
]
|
||||
};
|
||||
let mut expected = records_spine();
|
||||
expected.extend(fixed("Month", "Feb"));
|
||||
expected.extend(fixed("Region", "North"));
|
||||
expected.extend(fixed("_Measure", "Revenue"));
|
||||
expected.extend(["Product", "Customer"].map(|c| ViewStep::Axis {
|
||||
category: c.into(),
|
||||
axis: Axis::None,
|
||||
}));
|
||||
assert_eq!(drill_steps(&key, &none_cats), expected);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -17,7 +17,9 @@ indexmap = { version = "2", features = ["serde"] }
|
||||
pest = "2.9.2"
|
||||
pest_derive = "2.9.2"
|
||||
serde = { version = "1", features = ["derive"] }
|
||||
serde_json = "1"
|
||||
# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate,
|
||||
# because each build stands alone (improvise-6yvg, context/code/protocol.md).
|
||||
serde_json = { version = "1", features = ["float_roundtrip"] }
|
||||
|
||||
# The browser backend. Only the wasm build pulls these in; the native
|
||||
# binary never sees them (improvise-i34).
|
||||
|
||||
@@ -205,6 +205,28 @@ mod tests {
|
||||
assert_eq!(m2.model.name, "Budget");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_legacy_json_workbook_loads_its_numbers_bit_for_bit() {
|
||||
use crate::storage::{MemoryFs, Storage};
|
||||
const LOSSY: f64 = 0.248_157_142_857_142_85;
|
||||
let key = coord(&[("Type", "Food"), ("Month", "Jan")]);
|
||||
let mut wb = two_cat_model();
|
||||
wb.model.set_cell(key.clone(), CellValue::Number(LOSSY));
|
||||
let json = serde_json::to_string(&wb).unwrap();
|
||||
assert!(
|
||||
json.contains("0.24815714285714285"),
|
||||
"the file holds the exact text"
|
||||
);
|
||||
let fs = MemoryFs::new();
|
||||
let path = std::path::Path::new("legacy.improv");
|
||||
fs.write(path, json.as_bytes()).unwrap();
|
||||
let loaded = super::load_from(&fs, path).unwrap();
|
||||
let Some(CellValue::Number(n)) = loaded.model.get_cell(&key) else {
|
||||
panic!("the cell must load as a number");
|
||||
};
|
||||
assert_eq!(n.to_bits(), LOSSY.to_bits(), "loaded {n:?}, not {LOSSY:?}");
|
||||
}
|
||||
|
||||
// ── save/load roundtrip via file ────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -14,7 +14,8 @@ serde = { version = "1", features = ["derive"] }
|
||||
# `char`, and the TUI has always measured it this way.
|
||||
unicode-width = "^0.2"
|
||||
# Only for `variants`, which is off unless a test build asks for it.
|
||||
serde_json = { version = "1", optional = true }
|
||||
# float_roundtrip because every normal serde_json dep carries it (improvise-dz8c).
|
||||
serde_json = { version = "1", optional = true, features = ["float_roundtrip"] }
|
||||
|
||||
[features]
|
||||
# `variants` reaches OUT of this crate: improvise-command bounds its own
|
||||
|
||||
@@ -324,11 +324,12 @@ pub struct HelpPage {
|
||||
}
|
||||
|
||||
pub use key_row::KeyRow;
|
||||
use key_row::keys;
|
||||
|
||||
/// The one door to a keys row: its fields are private to this module, and its
|
||||
/// only constructor looks the keys up.
|
||||
mod key_row {
|
||||
use super::{ModeKey, key_label, keys_for, prefixed_labels};
|
||||
use super::{HelpLine, ModeKey, key_label, keys_for, prefixed_labels};
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct KeyRow {
|
||||
@@ -347,7 +348,7 @@ mod key_row {
|
||||
|
||||
/// An unbound command yields no keys and no row, so the page never
|
||||
/// advertises a key that does not exist.
|
||||
pub(super) fn looked_up(
|
||||
fn looked_up(
|
||||
mode: &ModeKey,
|
||||
command: &str,
|
||||
args: Option<&[String]>,
|
||||
@@ -361,17 +362,18 @@ mod key_row {
|
||||
(!keys.is_empty()).then_some(Self { keys, description })
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// A keys row, looked up in `mode`.
|
||||
fn keys(
|
||||
mode: &ModeKey,
|
||||
command: &'static str,
|
||||
args: Option<&[&str]>,
|
||||
description: &'static str,
|
||||
) -> Option<HelpLine> {
|
||||
let args: Option<Vec<String>> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect());
|
||||
KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys)
|
||||
/// A keys row, looked up in `mode`: the one caller of [`KeyRow::looked_up`],
|
||||
/// and the lookup `pages` hands every builder (improvise-ssoe).
|
||||
pub(super) fn keys(
|
||||
mode: &ModeKey,
|
||||
command: &'static str,
|
||||
args: Option<&[&str]>,
|
||||
description: &'static str,
|
||||
) -> Option<HelpLine> {
|
||||
let args: Option<Vec<String>> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect());
|
||||
KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys)
|
||||
}
|
||||
}
|
||||
|
||||
/// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap.
|
||||
@@ -1021,6 +1023,29 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// improvise-ssoe: a keys row drawn but never asked for through the builders'
|
||||
/// parameter is invisible to every guard. See `context/test-evidence/protocol.md`.
|
||||
#[test]
|
||||
fn every_keys_row_drawn_is_one_the_builders_asked_for() {
|
||||
let drawn: Vec<HelpLine> = pages()
|
||||
.into_iter()
|
||||
.flat_map(|page| page.lines)
|
||||
.filter(|line| matches!(line, HelpLine::Keys(_)))
|
||||
.collect();
|
||||
// Content, in order, not a count: a bypass that also drops a real row
|
||||
// keeps the count and changes this (improvise-aszt).
|
||||
let asked: Vec<HelpLine> = help_bindings()
|
||||
.iter()
|
||||
.filter_map(|(mode, command, args, description)| {
|
||||
let args: Option<Vec<&str>> = args
|
||||
.as_ref()
|
||||
.map(|a| a.iter().map(String::as_str).collect());
|
||||
keys(mode, command, args.as_deref(), description)
|
||||
})
|
||||
.collect();
|
||||
assert_eq!(drawn, asked);
|
||||
}
|
||||
|
||||
/// A row whose lookup finds no key is dropped silently, so a broken lookup
|
||||
/// looks like a shorter page. See `context/test-evidence/protocol.md`.
|
||||
#[test]
|
||||
|
||||
@@ -619,6 +619,11 @@ enum GridMove {
|
||||
Page(i32),
|
||||
}
|
||||
|
||||
/// Rows one `page-scroll` step moves: three quarters of the window, at least one.
|
||||
fn page_rows(window_rows: usize) -> i32 {
|
||||
(window_rows as i32 * 3 / 4).max(1)
|
||||
}
|
||||
|
||||
fn grid_move(state: &ClientState, kind: GridMove) -> Option<Vec<Command>> {
|
||||
let GridExtent { rows, cols } = state.grid.extent;
|
||||
let (window_rows, window_cols) = state.window;
|
||||
@@ -641,7 +646,7 @@ fn grid_move(state: &ClientState, kind: GridMove) -> Option<Vec<Command>> {
|
||||
GridMove::ToEnd(true) => (row_max as usize, cursor.selected.1),
|
||||
GridMove::ToEnd(false) => (cursor.selected.0, col_max as usize),
|
||||
GridMove::Page(dir) => {
|
||||
let delta = (window_rows as i32 * 3 / 4).max(1) * dir;
|
||||
let delta = page_rows(window_rows) * dir;
|
||||
((row + delta).clamp(0, row_max) as usize, cursor.selected.1)
|
||||
}
|
||||
};
|
||||
@@ -2362,47 +2367,90 @@ mod tests {
|
||||
#[test]
|
||||
fn every_grid_move_lands_where_its_binding_says() {
|
||||
let keymap = Keymaps::shared().get(&ModeKey::Normal).expect("Normal");
|
||||
let mut moves = Vec::new();
|
||||
bound_commands(keymap, &mut moves);
|
||||
// A move's destination is its own args; a jump's is its name.
|
||||
let mut expected: Vec<(String, Vec<String>, (usize, usize))> = moves
|
||||
.into_iter()
|
||||
.filter(|(name, _)| name == "move-selection")
|
||||
.map(|(name, args)| {
|
||||
let d = |i: usize| args[i].parse::<isize>().expect("a delta");
|
||||
let to = ((1 + d(0)) as usize, (1 + d(1)) as usize);
|
||||
(name, args, to)
|
||||
})
|
||||
.collect();
|
||||
for (name, to) in [
|
||||
("jump-first-row", (0, 1)),
|
||||
("jump-last-row", (2, 1)),
|
||||
("jump-first-col", (1, 0)),
|
||||
("jump-last-col", (1, 2)),
|
||||
] {
|
||||
expected.push((name.to_string(), Vec::new(), to));
|
||||
}
|
||||
let mut pressed = 0;
|
||||
for (name, args, to) in expected {
|
||||
let wanted = (!args.is_empty()).then_some(args.as_slice());
|
||||
for sequence in sequences_running(keymap, &name, wanted) {
|
||||
let mut bound = Vec::new();
|
||||
bound_commands(keymap, &mut bound);
|
||||
bound.sort();
|
||||
bound.dedup();
|
||||
// Far from every edge of a grid far larger than any step, so a landing
|
||||
// shows magnitude as well as sign (improvise-5kdr); a jump's is its name.
|
||||
const START: (usize, usize) = (10, 10);
|
||||
const WINDOW: (usize, usize) = (8, 8);
|
||||
let d = |a: &String| a.parse::<isize>().expect("a delta");
|
||||
let at = |base: usize, by: isize| base.checked_add_signed(by).expect("inside the grid");
|
||||
let page = page_rows(WINDOW.0) as isize;
|
||||
let destination = |name: &str, args: &[String]| match name {
|
||||
"move-selection" => Some((at(START.0, d(&args[0])), at(START.1, d(&args[1])))),
|
||||
"scroll-rows" => Some((at(START.0, d(&args[0])), START.1)),
|
||||
"page-scroll" => Some((at(START.0, page * d(&args[0])), START.1)),
|
||||
"jump-first-row" => Some((0, START.1)),
|
||||
"jump-last-row" => Some((20, START.1)),
|
||||
"jump-first-col" => Some((START.0, 0)),
|
||||
"jump-last-col" => Some((START.0, 20)),
|
||||
_ => None,
|
||||
};
|
||||
let mut unstated = Vec::new();
|
||||
let mut unpressed = Vec::new();
|
||||
let mut pressed = std::collections::BTreeSet::new();
|
||||
for (name, args) in &bound {
|
||||
let to = destination(name, args);
|
||||
let sequences = sequences_running(keymap, name, Some(args.as_slice()));
|
||||
if to.is_some() && sequences.is_empty() {
|
||||
unpressed.push(format!("{name} {args:?}"));
|
||||
}
|
||||
for sequence in sequences {
|
||||
let mut state = furnished(AppMode::Normal);
|
||||
state.grid.extent = GridExtent { rows: 21, cols: 21 };
|
||||
state.window = WINDOW;
|
||||
*state.view.cursor_mut() = crate::ViewCursor {
|
||||
selected: START,
|
||||
row_offset: 5,
|
||||
col_offset: 5,
|
||||
};
|
||||
let view = state.view.active_view.clone();
|
||||
for key in &sequence {
|
||||
if let KeyOutcome::Local(commands) = resolve(&state, *key) {
|
||||
commands.iter().for_each(|c| state.fold(c));
|
||||
}
|
||||
}
|
||||
assert_eq!(
|
||||
state.view.cursor().selected,
|
||||
to,
|
||||
"{sequence:?} runs `{name} {args:?}` from (1,1)"
|
||||
);
|
||||
pressed += 1;
|
||||
let landed = state.view.cursor().selected;
|
||||
if to.is_some() {
|
||||
pressed.insert(name.as_str());
|
||||
}
|
||||
match to {
|
||||
Some(to) => {
|
||||
assert_eq!(
|
||||
landed, to,
|
||||
"{sequence:?} runs `{name} {args:?}` from {START:?}"
|
||||
)
|
||||
}
|
||||
None if state.view.active_view == view && landed != START => {
|
||||
unstated.push(format!("{name} {args:?} -> {landed:?}"))
|
||||
}
|
||||
None => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
pressed >= 14,
|
||||
"only {pressed} keys pressed; the walk went blind"
|
||||
unpressed.is_empty(),
|
||||
"stated and never pressed: {unpressed:?}"
|
||||
);
|
||||
assert!(
|
||||
unstated.is_empty(),
|
||||
"these move the cursor and nothing states where to: {unstated:?}"
|
||||
);
|
||||
let every = [
|
||||
"jump-first-col",
|
||||
"jump-first-row",
|
||||
"jump-last-col",
|
||||
"jump-last-row",
|
||||
"move-selection",
|
||||
"page-scroll",
|
||||
"scroll-rows",
|
||||
];
|
||||
assert_eq!(
|
||||
pressed.into_iter().collect::<Vec<_>>(),
|
||||
every,
|
||||
"each kind is pressed"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -889,44 +889,6 @@ fn a_yanked_number_crosses_the_serialized_wire_exactly() {
|
||||
);
|
||||
}
|
||||
|
||||
/// The two tests around this one share a feature-unified build, so neither can
|
||||
/// see ONE frame-parsing crate lacking `float_roundtrip`. Evidence file.
|
||||
#[test]
|
||||
fn every_build_that_parses_a_frame_enables_float_roundtrip() {
|
||||
for (krate, target) in [
|
||||
("improvise-wasm-client", Some("wasm32-unknown-unknown")),
|
||||
("improvise-worker-server", Some("wasm32-unknown-unknown")),
|
||||
("improvise-ws-server", None),
|
||||
] {
|
||||
let mut cargo = std::process::Command::new(env!("CARGO"));
|
||||
cargo.current_dir(env!("CARGO_MANIFEST_DIR")).args([
|
||||
"tree",
|
||||
"--locked",
|
||||
"--edges",
|
||||
"normal,features",
|
||||
"--invert",
|
||||
"serde_json",
|
||||
"--package",
|
||||
krate,
|
||||
]);
|
||||
if let Some(target) = target {
|
||||
cargo.args(["--target", target]);
|
||||
}
|
||||
let out = cargo.output().expect("cargo tree must run");
|
||||
assert!(
|
||||
out.status.success(),
|
||||
"{krate}: cargo tree failed: {}",
|
||||
String::from_utf8_lossy(&out.stderr)
|
||||
);
|
||||
let tree = String::from_utf8(out.stdout).expect("cargo tree prints UTF-8");
|
||||
assert!(
|
||||
tree.contains(r#"serde_json feature "float_roundtrip""#),
|
||||
"{krate} parses frames without float_roundtrip, so its numbers land \
|
||||
one ULP off:\n{tree}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// A typed number is parsed exactly in the client and re-parsed from JSON by
|
||||
/// the host. Evidence file.
|
||||
#[test]
|
||||
|
||||
@@ -0,0 +1,61 @@
|
||||
//! A number read through serde_json must be the number written (improvise-6yvg,
|
||||
//! improvise-dz8c). The rule and its derivation: `context/code/protocol.md`.
|
||||
|
||||
use serde_json::Value;
|
||||
|
||||
/// Every NORMAL serde_json dependency in the workspace, and those lacking
|
||||
/// `float_roundtrip`, each spelled `package (target)`.
|
||||
fn lossy_json_dependencies(metadata: &Value) -> (Vec<String>, Vec<String>) {
|
||||
let mut checked = vec![];
|
||||
let mut lossy = vec![];
|
||||
let packages = metadata["packages"]
|
||||
.as_array()
|
||||
.expect("metadata lists packages");
|
||||
for package in packages {
|
||||
let name = package["name"].as_str().expect("a package has a name");
|
||||
let deps = package["dependencies"]
|
||||
.as_array()
|
||||
.expect("a package lists deps");
|
||||
for dep in deps {
|
||||
if dep["name"] != "serde_json" || !dep["kind"].is_null() {
|
||||
continue;
|
||||
}
|
||||
let target = dep["target"].as_str().unwrap_or("all targets");
|
||||
let spelled = format!("{name} ({target})");
|
||||
let features = dep["features"].as_array().expect("a dep lists features");
|
||||
if !features.iter().any(|f| f == "float_roundtrip") {
|
||||
lossy.push(spelled.clone());
|
||||
}
|
||||
checked.push(spelled);
|
||||
}
|
||||
}
|
||||
(checked, lossy)
|
||||
}
|
||||
|
||||
/// Derived from `cargo metadata`, not listed: a new crate that can parse JSON
|
||||
/// is covered the day it declares the dependency. Evidence file.
|
||||
#[test]
|
||||
fn every_crate_that_can_parse_json_enables_float_roundtrip() {
|
||||
let out = std::process::Command::new(env!("CARGO"))
|
||||
.current_dir(env!("CARGO_MANIFEST_DIR"))
|
||||
.args(["metadata", "--no-deps", "--format-version", "1"])
|
||||
.output()
|
||||
.expect("cargo metadata must run");
|
||||
assert!(
|
||||
out.status.success(),
|
||||
"cargo metadata failed: {}",
|
||||
String::from_utf8_lossy(&out.stderr)
|
||||
);
|
||||
let metadata: Value = serde_json::from_slice(&out.stdout).expect("metadata is JSON");
|
||||
let (checked, lossy) = lossy_json_dependencies(&metadata);
|
||||
assert!(
|
||||
checked.len() >= 2,
|
||||
"the derivation found {checked:?}: too few to be reading the metadata right"
|
||||
);
|
||||
assert!(
|
||||
lossy.is_empty(),
|
||||
"these parse JSON numbers with serde_json's lossy default, one ULP off \
|
||||
for roughly one decimal in seven; add features = [\"float_roundtrip\"] \
|
||||
to each crate's own manifest: {lossy:?}"
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user