Merge remote-tracking branch 'gh/main' into fix/6oy4-float-roundtrip
# Conflicts: # context/test-evidence/clients.md
This commit is contained in:
Edward Langley
@@ -95,17 +95,14 @@ From `design-principles.md` §7 and `doctrine/2`:
|
||||
|
||||
## Verifying
|
||||
|
||||
Run the gate **CI runs**, flags and all. Clippy exits 0 on warnings, so without
|
||||
`-D warnings` this file was green on exactly the input CI is red on — a check
|
||||
that cannot fail in the direction the defect lies.
|
||||
|
||||
```bash
|
||||
cargo test --workspace --no-fail-fast # fail-fast hides the blast radius
|
||||
cargo clippy --workspace --all-targets -- -D warnings
|
||||
cargo fmt --all --check
|
||||
python3 scripts/check-comment-runs.py # the two-line comment cap
|
||||
python3 scripts/check-comment-runs.py --check-evidence # a DIFFERENT question
|
||||
```
|
||||
Run the gate **CI runs**, flags and all. The list is
|
||||
`.agents/skills/wave/agent-rules.md`, "Gates", and it is kept only there. Run
|
||||
it from that file rather than from memory or from an older copy. A second
|
||||
copy of that list lived here and drifted: it said `cargo fmt --all --check`,
|
||||
which reaches only one of this repo's two workspace roots, and it left out
|
||||
`direnv exec .`, `scripts/checkers.sh` and `scripts/node-test.sh`
|
||||
(improvise-vjus). Flags matter. Clippy exits 0 on warnings, so without
|
||||
`-D warnings` a clippy run was green on exactly the input CI is red on.
|
||||
|
||||
**`--check-evidence` is additive**, so it answers the comment cap too and both
|
||||
verdicts print. It did not always: the flag used to *select*, so the flagged
|
||||
|
||||
@@ -74,6 +74,12 @@ direnv exec . ./scripts/node-test.sh 'web/tests/*.test.js'
|
||||
`cargo check --target wasm32-unknown-unknown -p <crate>` **for each crate**.
|
||||
CI checks each one separately, so feature unification cannot hide a leak.
|
||||
- If you touch `web/` or `harness/`, run the Playwright suite in `harness/`.
|
||||
- **If you change the grid's row or column count, also grep
|
||||
`harness/fixtures/*.improv` and `harness/tests/*.spec.js`.** On PR #176 a
|
||||
Playwright spec pinned the old collapsed-group row count. `cargo test` cannot
|
||||
see that.
|
||||
- **A PR that conflicts with its base gets no CI run.** So anything "left to
|
||||
CI" is unchecked until the PR is mergeable (PR #177).
|
||||
- **Measure your own baseline** before changing anything, and report it with
|
||||
the machine it was measured on. A figure from the brief is a reference, not
|
||||
your baseline.
|
||||
|
||||
Symlink
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/code-review
|
||||
@@ -1,139 +0,0 @@
|
||||
---
|
||||
name: code-review
|
||||
description: Review a pull request or diff against this repository's own written architecture and testing doctrine — the Cmd/Effect split, exhaustive enums over AppMode/Axis/CategoryKind, crate layering, the CellKey/layout/persistence invariants, and when a green suite is not evidence. Use when reviewing any change to this repo, and before pushing your own work.
|
||||
---
|
||||
|
||||
# Code review
|
||||
|
||||
This repo has already written down what "correct shape" means, in more detail
|
||||
than most. A review here is mostly checking a change against that, rather than
|
||||
against generic standards.
|
||||
|
||||
Two passes, in order:
|
||||
|
||||
1. **Correctness** — does the change do what it claims? Wrong logic, missed
|
||||
edge cases, error paths that swallow failures. Ordinary review judgement.
|
||||
2. **Conformance** — does it fit the design? That is what the rest of this
|
||||
file is for, and it is the part a reviewer who has not read `context/`
|
||||
will miss.
|
||||
|
||||
## Read the sources, don't trust this file
|
||||
|
||||
This skill is an index. The rules live in files that change, and restating
|
||||
them here would let the copy drift:
|
||||
|
||||
| Source | What it holds |
|
||||
|---|---|
|
||||
| `context/design-principles.md` | The "why" — functional-first, polymorphism over conditionals, correctness by construction, layering, §7 testing doctrine |
|
||||
| `context/repo-map.md` | The "where", plus **numbered gotchas** — read the ones your diff touches |
|
||||
| `prolog-sessions/kb.pl` | Executable knowledge: `invariant/2`, `doctrine/2`, `forbidden/2`, `decision/3`, and prior `work_done/3` |
|
||||
| `AGENTS.md` | Working rules: bug-first tests, `Option`/`Result`/`Box` at the edges, merges over rebases |
|
||||
|
||||
`kb.pl` is the highest-value and least-obvious of these. Grep it before
|
||||
reviewing: `grep -n "invariant(\|doctrine(\|decision(" prolog-sessions/kb.pl`.
|
||||
A `decision/3` fact often explains why the thing you are about to flag looks
|
||||
the way it does.
|
||||
|
||||
## What to check
|
||||
|
||||
**Command/effect split.** `Cmd::execute` takes `&CmdContext` and returns
|
||||
`Vec<Box<dyn Effect>>`. Mutation happens only in `Effect::apply`. A command
|
||||
reaching for mutable state is the single most common shape error.
|
||||
|
||||
**New effects.** `kind()` has no default, so the compiler asks — but check
|
||||
the classification is right, not just present (`doctrine(derive_then_verify)`
|
||||
records naive extraction mis-classifying three effects). Note there is no
|
||||
`changes_mode()` any more: mode settling is observed after the batch, not
|
||||
predicted per type (gotcha 31, improvise-toc). Don't reintroduce a static
|
||||
flag for a conditional behaviour.
|
||||
|
||||
**Exhaustive matches.** `AppMode`, `Axis`, `BinOp`, `CategoryKind` and friends
|
||||
should match exhaustively — a `_ =>` arm is how adding a variant silently
|
||||
skips a site. When a new variant shares one property with an existing variant
|
||||
but not another, name both as predicates and route call sites through the one
|
||||
they mean, rather than testing for the variant
|
||||
(`doctrine(two_questions_one_enum)`; `Axis::filters_data` /
|
||||
`is_user_assignable` are the worked example).
|
||||
|
||||
**Layering.** `forbidden/2` in `kb.pl` is the authority:
|
||||
`improvise-core` no UI/IO, `improvise-io` no UI/commands, `improvise-command`
|
||||
no ratatui/crossterm. A ratatui type in core is the usual violation — the fix
|
||||
is a semantic enum in core that the render layer maps (see `AxisColor`).
|
||||
|
||||
**`Option`/`Result`/`Box` at the boundaries.** Wrap at parse/lookup/IO, unwrap
|
||||
early, keep core logic on concrete values. Heap-container plumbing should be
|
||||
split from logic (`AGENTS.md`).
|
||||
|
||||
**Invariants the type system does not carry.** Check the relevant gotchas
|
||||
rather than reasoning from first principles — `CellKey` built through its
|
||||
constructors, `IndexMap` order being load-bearing for persistence, display
|
||||
rounding never fed back into eval, `app.layout` going stale mid-batch and
|
||||
needing `RebuildLayout` in front.
|
||||
|
||||
**Persistence.** The pest grammar is the source of truth; update it first.
|
||||
Wire tokens are not UI labels and must not follow them.
|
||||
|
||||
## Testing doctrine
|
||||
|
||||
From `design-principles.md` §7 and `doctrine/2`:
|
||||
|
||||
- **A bug fix needs a test that failed before it.** If the diff claims a fix
|
||||
with no such test, that is the finding. Ask whether it was run against the
|
||||
old code.
|
||||
- Prefer unit tests over integration/e2e; test the observable contract, not
|
||||
internal storage.
|
||||
- Suite stays **under 2 seconds**. Proptest stays at 256 cases — constrain the
|
||||
strategy instead of raising them.
|
||||
- `doctrine(check_the_checker)`: a new check should be proven by injecting the
|
||||
fault it targets. A check nobody has seen fail is indistinguishable from one
|
||||
that always passes.
|
||||
- `doctrine(populate_before_roundtrip)`: a roundtrip over a default-constructed
|
||||
value proves almost nothing.
|
||||
- `doctrine(binding_dump_diff)`: most keybindings have no direct test, so a
|
||||
green suite is **not** evidence for a keymap refactor — diff the dumped
|
||||
bindings.
|
||||
|
||||
## Verifying
|
||||
|
||||
Run the gate **CI runs**, flags and all. Clippy exits 0 on warnings, so without
|
||||
`-D warnings` this file was green on exactly the input CI is red on — a check
|
||||
that cannot fail in the direction the defect lies.
|
||||
|
||||
```bash
|
||||
cargo test --workspace --no-fail-fast # fail-fast hides the blast radius
|
||||
cargo clippy --workspace --all-targets -- -D warnings
|
||||
cargo fmt --all --check
|
||||
python3 scripts/check-comment-runs.py # the two-line comment cap
|
||||
python3 scripts/check-comment-runs.py --check-evidence # a DIFFERENT question
|
||||
```
|
||||
|
||||
**`--check-evidence` is additive**, so it answers the comment cap too and both
|
||||
verdicts print. It did not always: the flag used to *select*, so the flagged
|
||||
run skipped the cap and said nothing about having skipped it, and three
|
||||
separate agents hit that in one campaign — a local gate using only the flagged
|
||||
form went green while CI went red on a three-line doc comment (improvise-ijql).
|
||||
Run the bare form anyway when you only want the cap: it is 0.3s and needs no
|
||||
toolchain, where the flagged one needs a compiled workspace.
|
||||
|
||||
The knowledge base is checked in CI as code:
|
||||
|
||||
```bash
|
||||
nix shell nixpkgs#trealla --command tpl prolog-sessions/kb.pl \
|
||||
-g "(kb_consistent -> halt(0) ; (kb_report, halt(1)))"
|
||||
```
|
||||
|
||||
In a cloud container that needs one step first: `nix shell` resolves nixpkgs
|
||||
as a GitHub tarball, which is not served there. Run
|
||||
`scripts/nix-seed-flake-inputs.sh` to seed the locked inputs over git (it does
|
||||
not modify `flake.lock`), after which the command works. Details and the two
|
||||
alternatives are in `context/cloud-agents.md`. If you would rather not spend
|
||||
the time, review `kb.pl` edits by reading and let CI run the check.
|
||||
|
||||
## Closing the loop
|
||||
|
||||
If the review surfaces a durable fact — a decision, a new invariant, a method
|
||||
that worked — add it to `prolog-sessions/kb.pl` as `decision/3`,
|
||||
`invariant/2` or `doctrine/2`. If it changes where something lives or how it
|
||||
behaves, update `context/repo-map.md`. Those two files and
|
||||
`context/design-principles.md` are maintained by hand and are the reason this
|
||||
review is cheap next time.
|
||||
Symlink
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/kb
|
||||
@@ -1,118 +0,0 @@
|
||||
---
|
||||
name: kb
|
||||
description: Read and write the Prolog knowledge base at prolog-sessions/kb.pl. Use when adding a doctrine, invariant, decision or work_done fact, correcting or retiring one, or querying the formal model. Never edit that file by hand and never write it through the Prolog MCP.
|
||||
---
|
||||
|
||||
# Writing the knowledge base
|
||||
|
||||
`prolog-sessions/kb.pl` is executable knowledge, gated by CI. **`scripts/kb` is
|
||||
the only thing that writes it.** `tpl` is assumed on `PATH` from the dev shell.
|
||||
|
||||
```
|
||||
scripts/kb check # kb_consistent, exactly what CI runs
|
||||
scripts/kb check --with <d.pl># does the model accept a design you have not built?
|
||||
scripts/kb count # clauses per keyed predicate
|
||||
scripts/kb query '<goal>' # run a goal; exit 1 if it fails
|
||||
scripts/kb add <delta.pl> # add clauses; refuses if a key already exists
|
||||
scripts/kb replace <delta.pl> # add, overwriting existing keys
|
||||
scripts/kb rm <keys.pl> # remove by key; refuses if a key is absent
|
||||
scripts/kb fmt # canonical form (idempotent)
|
||||
```
|
||||
|
||||
A delta is an ordinary Prolog file of clauses. For `rm`, a key is a clause head
|
||||
with the last argument left as `_`:
|
||||
|
||||
```prolog
|
||||
doctrine(a_name_that_went_false,_).
|
||||
```
|
||||
|
||||
## Designing against the model
|
||||
|
||||
Before implementing a ticket, state the design as facts and ask whether the
|
||||
model accepts it:
|
||||
|
||||
```prolog
|
||||
crate(improvise_report,'crates/improvise-report','render a View as a report').
|
||||
crate_depends_on(improvise_report,improvise_core).
|
||||
layer(report,improvise_report).
|
||||
```
|
||||
|
||||
`scripts/kb check --with design.pl` merges those into a throwaway candidate and
|
||||
runs `kb_consistent` over the union. Nothing is written either way.
|
||||
|
||||
It derives consequences you would not enumerate: one wrong dependency edge
|
||||
returns six `layering_inverted` violations through the transitive closure. It
|
||||
also catches schema mistakes — `layer/2` is `(layer, crate)`, and the wrong
|
||||
order comes back as `layer_names_unknown_crate` rather than being accepted.
|
||||
|
||||
What the model can refuse today: layering and dependency direction
|
||||
(`illegal_dependency`, `layering_inverted`, `may_use`), `ViewState` membership
|
||||
(`portable`, `snapshot_safe`, `view_state_admits`, `field_misplaced_in_view_state`),
|
||||
record obligations, and functional-key conflicts. A design touching those areas
|
||||
is worth stating; one that touches none of them will be accepted vacuously, so
|
||||
extend the model rather than trusting the green.
|
||||
|
||||
## Rules
|
||||
|
||||
**Write a fact when you learn it.** There is no reason to batch.
|
||||
|
||||
**Verify the claim against the tree before writing it.** A clause stating the
|
||||
negation of what the code does is worse than no clause
|
||||
(`doctrine(a_stale_gotcha_asserts_the_negation)`).
|
||||
|
||||
**If the NAME went false, retire it** — `rm` the old key, `add` the new one, and
|
||||
say in the new clause that it was renamed. A reader who trusts a name never
|
||||
reads the body.
|
||||
|
||||
**Put the argument in the clause.** Doctrine and decision text is where worked
|
||||
examples and history belong; prose docs should stay operational.
|
||||
|
||||
## Traps
|
||||
|
||||
- **`assertz` does not work here.** A consulted predicate is static;
|
||||
`assertz`/`retract` raise `permission_error(modify, static_procedure)`. This
|
||||
is why the tool reads clauses as data.
|
||||
- **`kb_consistent` does not check everything.** A clause name cited *inside
|
||||
another clause's string* is unchecked, so retiring a clause can dangle every
|
||||
clause referring to it (`improvise-2f97`).
|
||||
- **Reading through the MCP is fine.** `loadSession` at session start is still
|
||||
how a session gets the model in hand; only writing moved.
|
||||
|
||||
Every write checks three things before it replaces the file: the result reads
|
||||
back as intended, it still holds every clause **HEAD** has, and `kb_consistent`
|
||||
passes. So a bad delta leaves the file untouched. `count` before and after is
|
||||
still the cheapest evidence a write did what you meant.
|
||||
|
||||
## When a write is refused
|
||||
|
||||
Exit codes: `0` done, `1` refused, `2` usage, `3` HEAD's `kb.pl` could not be
|
||||
read, so nothing was compared, `127` no `tpl`. Only `1` is a verdict.
|
||||
|
||||
**"clauses HEAD has are gone, with no kb rm recorded"** means the working-tree
|
||||
`kb.pl` is short of the committed one. Your delta did not cause it. The file
|
||||
was already damaged, by a killed write, a bad merge resolution or an edit. The
|
||||
refusal lists the clauses that are missing. The check counts clauses **per
|
||||
key**, so losing one of two clauses that share a key is caught too.
|
||||
|
||||
- **Recover:** `git show HEAD:prolog-sessions/kb.pl > prolog-sessions/kb.pl`,
|
||||
then re-run your `add`/`replace`/`rm` deltas.
|
||||
- **Do not commit the short file to get past the refusal.** HEAD is what the
|
||||
check compares against, so a committed loss becomes the new floor and is
|
||||
never reported again.
|
||||
- **To remove a clause on purpose, use `kb rm`.** It records the removal in a
|
||||
per-worktree ledger (`git rev-parse --git-path kb-removed.pl`, never
|
||||
committed), and that record is what lets later writes accept the missing
|
||||
clause. A hand deletion has no record, so it is refused.
|
||||
|
||||
The ledger vouches only for the exact `kb.pl` file the tool last wrote. Any
|
||||
rewrite outside the tool voids it, including a restore, `git checkout`, `cp`
|
||||
or an editor. After an `rm`, a restore therefore also undoes the `rm`, and you
|
||||
must redo it. That is deliberate. Otherwise the abandoned `rm` would go on
|
||||
excusing a later, accidental loss of the same clause.
|
||||
|
||||
A killed write can leave `kb.pl.cand.*` files and a `kb.pl.lock` directory in
|
||||
`prolog-sessions/`. Git ignores them. The lock makes the next write wait 30
|
||||
seconds and then fail, so delete it if nothing else is writing.
|
||||
|
||||
`context/code/kb-tool.md` has the argument. `scripts/kb-test.sh` proves each
|
||||
refusal by injecting the fault it names.
|
||||
Symlink
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/pr-watch
|
||||
@@ -1,43 +0,0 @@
|
||||
---
|
||||
name: pr-watch
|
||||
description: Wait for a GitHub PR's checks to finish before merging or reporting on it. Use whenever you need to know whether a PR is green. Do not hand-roll a polling loop over gh pr view.
|
||||
---
|
||||
|
||||
# Waiting on a PR
|
||||
|
||||
```
|
||||
scripts/pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec>]
|
||||
```
|
||||
|
||||
```
|
||||
exit 0 every check COMPLETED and none failed
|
||||
exit 1 a check failed, or the PR is CONFLICTING
|
||||
exit 2 the head moved during the wait, or is not --expect
|
||||
exit 3 timed out with checks still running
|
||||
exit 4 usage or environment error
|
||||
```
|
||||
|
||||
**Pass `--expect <sha>` whenever you know which commit you are asking about.**
|
||||
It is the difference between "this PR is green" and "the thing I pushed is
|
||||
green", and a PR here was once merged at a head that predated its own fix.
|
||||
|
||||
## Do not write the loop yourself
|
||||
|
||||
Three signals look like "finished" and are not:
|
||||
|
||||
- **"nothing pending"** — also true when no check has registered yet. Require a
|
||||
**non-zero total** as well.
|
||||
- **`conclusion == null`** — `conclusion` is a *string*, empty while running.
|
||||
Test `status == "COMPLETED"`.
|
||||
- **a verdict from before the head moved** — re-check the head, or `--expect`.
|
||||
|
||||
`gh` is shell-aliased to a different tool in these shells; `pr-watch` locates a
|
||||
real one itself. Do not reintroduce a bare `gh` call in a script here.
|
||||
|
||||
## Before deciding a PR is stuck
|
||||
|
||||
Check suites can take several minutes to appear at all, and a Copilot review
|
||||
takes about six. Compare `date -u` against the GitHub timeline rather than a
|
||||
felt interval, and do not close and reopen a PR to unstick it.
|
||||
|
||||
`scripts/pr-watch-test.sh` drives every case from fixtures.
|
||||
Generated
+1
@@ -845,6 +845,7 @@ dependencies = [
|
||||
"improvise-protocol",
|
||||
"improvise-wasm-client",
|
||||
"indexmap",
|
||||
"libc",
|
||||
"pest",
|
||||
"pest_derive",
|
||||
"pest_meta",
|
||||
|
||||
@@ -59,6 +59,8 @@ enum_dispatch = "0.3.13"
|
||||
# natively, which is how its own tests already run.
|
||||
improvise-wasm-client = { path = "crates/improvise-wasm-client" }
|
||||
improvise-protocol = { path = "crates/improvise-protocol" }
|
||||
# `tests/headless_cli.rs` detaches the binary from any terminal with `setsid`.
|
||||
libc = "0.2"
|
||||
pest = "2.9.2"
|
||||
pest_derive = "2.9.2"
|
||||
pest_meta = "2.9.2"
|
||||
|
||||
@@ -203,13 +203,16 @@ improvise cmd 'formula Product "Total = Widget + Gadget"' -f model.improv
|
||||
TUI's status line, and stdout is left empty. A run that had nothing to report
|
||||
prints nothing, so `2>/dev/null` is the whole suppression story and there is
|
||||
no quiet flag.
|
||||
- **Exit 1** means a line did not parse, or an effect aborted the rest of its
|
||||
batch — a failed `:w`, say. **Exit 0 means neither of those happened**, and
|
||||
it does *not* promise every command did what it was asked: a refusal that
|
||||
only writes a status message (`add-item` naming a category that does not
|
||||
exist) is printed but still exits 0, because a status message carries no
|
||||
severity for the exit code to read. Check stderr as well as `$?` when it
|
||||
matters.
|
||||
- **Exit 1** means a line did not parse, or a command refused what it was
|
||||
asked — a failed `:w`, `add-item` naming a category that does not exist.
|
||||
A command that did its work and then had nowhere further to go — a
|
||||
`commit-cell-edit` on the last cell, whose advance stops — is not a refusal
|
||||
and exits 0. A few refusals are not yet classified and still exit 0
|
||||
(improvise-i3o5), so check stderr as well as `$?` when it matters.
|
||||
- **A run behaves as if its terminal were 80x24, whatever it runs in.** A page
|
||||
of rows is a real distance — `page-scroll` moves the cursor by one — so
|
||||
reading the actual window would make the same script write a different record
|
||||
in a different terminal, and a different one again in CI.
|
||||
- **A run that changed nothing does not rewrite the document.** Writing it back
|
||||
regardless silently normalises it — `1e20` becomes its twenty digits — which
|
||||
turns a hand-edited fixture into a no-op on the next run.
|
||||
|
||||
+76
-12
@@ -50,19 +50,16 @@ matches every other diagnostic `main.rs` already emits (`Saved to …`,
|
||||
**No `--quiet` flag.** Silence is never the default and `2>/dev/null` is the
|
||||
standard suppression; a flag would be a second knob with nothing asking for it.
|
||||
|
||||
## 3 — the exit code, and the hole left open deliberately
|
||||
## 3 — the exit code (superseded by §10)
|
||||
|
||||
Exit 1 on a parse error (pre-existing) or on `App::last_batch_aborted()`. Both
|
||||
are real flags read off state the code already keeps, so neither is a guess.
|
||||
|
||||
What this does **not** cover, stated rather than papered over: a refusal that
|
||||
only writes a status still exits 0. Closing it needs a severity beside
|
||||
`status_msg`, which lives in `ViewState` and is therefore a wire change
|
||||
(gotcha 25) reaching the browser client — out of proportion to the CLI, and
|
||||
filed as improvise-udt2, which names both rivals so the next attempt cannot
|
||||
rediscover them. Until then the README says plainly that exit 0 means "nothing
|
||||
aborted", not "everything worked", because a contract that over-promises is
|
||||
worse than a narrow one.
|
||||
This section used to say: exit 1 on a parse error or on
|
||||
`App::last_batch_aborted()`, and a refusal that only writes a status exits 0,
|
||||
because closing that needed a severity beside `status_msg` — a `ViewState`
|
||||
change, and so a wire change (gotcha 25). Both halves were wrong in a way worth
|
||||
keeping. The abort half made a *success* exit 1 (improvise-c5ip), and the
|
||||
severity half named the wrong home: the question an exit code asks is about one
|
||||
batch, not about a message, so it belongs beside `batch_aborted` on `App` and
|
||||
never reaches the wire. §10 is what replaced it.
|
||||
|
||||
## 4 — a run that changed nothing does not write
|
||||
|
||||
@@ -223,3 +220,70 @@ Guards: `every_argument_list_the_gate_accepts_the_run_accepts_too` enumerates
|
||||
cursor sits on no cell — the context in which the old `commit-cell-edit` arm
|
||||
refused. It lists no commands, so a new registration is covered the day it
|
||||
lands. It is backed by the process-level tests in `tests/headless_cli.rs`.
|
||||
|
||||
## 9 — the extent is fixed, because it reaches the document
|
||||
|
||||
A headless run used to read `crossterm::terminal::size()`, and the page size
|
||||
derived from it decides where `page-scroll` lands and therefore which record the
|
||||
next cursor-reading command writes (improvise-up8g). So one script wrote a
|
||||
different document in a different window, and with no tty — CI, cron — the read
|
||||
failed and left `App`'s default, so CI and a shell disagreed too. It was
|
||||
invisible until §7 let a script see its cursor.
|
||||
|
||||
`HEADLESS_EXTENT` in `main.rs` pins it to 80x24 — the size CI already had, so
|
||||
nothing that passed there moves. There is **no flag** to change it, for §2's
|
||||
reason: a second knob with nothing asking for it. A script that wants a
|
||||
particular row says so with `move-selection` or `jump-*`, which do not depend on
|
||||
any window.
|
||||
|
||||
What this does not close: the **TUI** still takes its extent from the terminal,
|
||||
and that is correct there, because the window really is an input to an
|
||||
interactive session. The ticket's wider claim — that a replayable log needs the
|
||||
extent in it (improvise-ilvl) — is about that log, which does not exist yet; a
|
||||
headless run has nothing to replay *except* its script, so pinning is the whole
|
||||
fix for this surface.
|
||||
|
||||
## 10 — the exit code asks whether the change happened, never whether a chain stopped
|
||||
|
||||
`App::last_batch_aborted()` answered the wrong question for an exit code, in
|
||||
both directions at once.
|
||||
|
||||
- **A success that exited 1** (improvise-c5ip). `commit-cell-edit` ends in
|
||||
`EnterAdvance`, which emits `AbortChain` when the cursor is already
|
||||
bottom-right or the grid is empty: *the advance's premise no longer holds*.
|
||||
The write had already landed and was saved, and the run exited 1 with
|
||||
nothing on stderr. Before §7 that was every scripted commit.
|
||||
- **A refusal that exited 0** (improvise-udt2). `AddItem` into a missing
|
||||
category writes the error and does not abort, because nothing after it
|
||||
depends on the item. So the flag never saw it.
|
||||
|
||||
An abort is a statement about the **chain**; a refusal is a statement about the
|
||||
**request**. `AbortChain` stops a chain without refusing anything, and
|
||||
`AddItem` refuses without stopping anything. The two coincide only for
|
||||
`AddCategory` and the file writes, which is why reading one for the other
|
||||
looked right for so long.
|
||||
|
||||
So `App` carries a second per-batch outcome, `batch_refused`, reset where
|
||||
`batch_aborted` is and written by exactly one method, `App::refuse(msg)`, which
|
||||
sets the status and the flag together — a refusal cannot write its message and
|
||||
forget the flag. Commands reach it through `effect::refuse`, the `Refuse`
|
||||
effect, beside `effect::set_status` for reports. `main.rs` exits 1 on
|
||||
`last_batch_refused()` and no longer reads `last_batch_aborted()` at all.
|
||||
|
||||
**Why not a severity on `status_msg`**, which udt2 proposed. A severity on the
|
||||
message would live in `ViewState`, so it would be portable state sent to every
|
||||
client in a `Snapshot`, and it would go **stale**: every one of the ~60 writers
|
||||
of `status_msg` would have to reset it or leave the previous message's severity
|
||||
in place. A per-batch flag cannot go stale, because `apply_effects` clears it
|
||||
before every batch, and it is not in `ViewState`, so nothing on the wire
|
||||
carries it or has to agree with it. The TUI colouring the ticket hoped for is
|
||||
still possible later, and would want the severity *on the message*; that is a
|
||||
separate change with a separate staleness problem.
|
||||
|
||||
**The classification is per writer, and it is not total.** Every refusal in
|
||||
`ui/effect.rs` and in `cmd/{commit,fit,grid,mode,search,text_buffer}.rs` goes
|
||||
through `refuse`. Those still written with `set_status` exit 0 as before, and
|
||||
are listed in improvise-i3o5. `q` on a dirty document is deliberately left a
|
||||
report: headless, its text is false — the CLI saves anyway (improvise-ep5o).
|
||||
The two rivals udt2 named both stay green: a successful `:w` and a successful
|
||||
`fit-measure` report through `set_status` and exit 0.
|
||||
|
||||
+48
-1
@@ -990,7 +990,9 @@ Its one consumer is `Host::save`, where the batch is literally `[Save]` and so
|
||||
improvise-368h). **Naming the real call site matters** because that identity is
|
||||
what makes the flag readable at all: over a longer batch it says that *something*
|
||||
refused, never which. `App::last_batch_aborted` is the accessor and carries the
|
||||
same caveat.
|
||||
same caveat. It is **not** the refusal flag: `AbortChain` aborts without
|
||||
refusing and `AddItem` refuses without aborting, so "did the request happen" is
|
||||
`App::last_batch_refused`, written only by `App::refuse` (`context/code/cli.md` §10).
|
||||
|
||||
### `App::new` recomputes formulas before the first layout
|
||||
|
||||
@@ -3076,3 +3078,48 @@ and every entry, so *any* append reframes and throws the cursor to the origin
|
||||
although nothing was renumbered. Placing the cursor masks that at this call
|
||||
site. Distinguishing "rows appended" from "rows renumbered" is a larger question
|
||||
and does not belong in a P1 data-loss fix.
|
||||
|
||||
## Paste lands on a record or nowhere (improvise-6p8q, improvise-l4mp)
|
||||
|
||||
`YankCell` and `PasteCell` carry a `CellKey` like every cell command, and in
|
||||
records mode the cursor's key is the **synthetic** `(_Index, _Dim)` pair — a
|
||||
row index and a column index (gotcha 1). Both commands used it as though it
|
||||
named a cell. `yy` evaluated it and got `None` on every records cell; `p` wrote
|
||||
a *new* cell keyed `_Dim=|3|, _Index=|0|`, which `format_md` then persisted into
|
||||
`## Data`. One keystroke corrupted the saved document, and the browser, which
|
||||
composes its paste against the projected `CellId::Record`, never had the bug.
|
||||
|
||||
**Yank reads what the grid draws.** A synthetic key is answered by
|
||||
`GridLayout::records_value`, the one resolution of a records cell
|
||||
(improvise-tom1), so a coordinate column yanks its item as text and a measure
|
||||
column its stored value. That is also what the browser yanks, since
|
||||
improvise-dsxq made it copy `CellDisplay::value`, which the projection fills
|
||||
from the same call. One consequence is shared by both ends and is not decided
|
||||
here: a blank — a creating column, or a measure the record lacks — yanks
|
||||
`Text("")` where an empty pivot cell yanks `None`. Filtering it in the TUI alone
|
||||
would be a second derivation, so it is left to the producer (improvise-s1lx).
|
||||
|
||||
**Paste writes only where a value is stored.** A synthetic key resolves through
|
||||
`GridLayout::record_cell_key`; the coordinate columns and both creating columns
|
||||
store no value, so the paste is declined out loud rather than guessed at. A
|
||||
coordinate column is deliberately *not* a rename: renaming moves every measure
|
||||
cell in the row, a different intent from `SetCell`, and the browser does not
|
||||
offer it either (`project::cell_id` answers `None` there).
|
||||
|
||||
**Inside a drill the paste is declined, not staged.** Gotcha 19 asks the
|
||||
records branch to follow the commit path, whose drill arm stages the edit — but
|
||||
`pending_edits` holds *strings* and `CellValue`'s `Display` rounds a number to
|
||||
four places, so staging a pasted `1.23456789` would commit `1.2346`. Writing
|
||||
straight through would break the drill's atomicity. Declining is the only arm
|
||||
that loses nothing; a typed staging map is the real fix if drill paste is
|
||||
wanted.
|
||||
|
||||
**An aggregate is a key that leaves out a regular category the layout
|
||||
aggregates over** (improvise-l4mp). That is asked of the *key*, not of the view,
|
||||
so `paste Region=West _Measure=Revenue Product=Gadget` from a script still
|
||||
writes where the current view happens to aggregate `Product` away: a key that
|
||||
names every coordinate is a record whatever the view shows. For the cursor's
|
||||
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`.
|
||||
|
||||
@@ -320,3 +320,78 @@ on a **closed** panel it opened it. The fixture closes every panel first,
|
||||
because on an open-and-focused panel the toggle also closes it and the test
|
||||
would pass against the bug. Rival: `execute` ignoring `target` and always
|
||||
toggling — fails this test for every panel.
|
||||
|
||||
### a_script_writes_the_same_record_whatever_terminal_ran_it
|
||||
|
||||
improvise-up8g. `run_headless_commands` read `crossterm::terminal::size()` into
|
||||
`App::term_{width,height}`, and `CmdContext::visible_rows` derives from the
|
||||
height, so `page-scroll 1 . hide-selected-row-item` over a 60-item row axis hid
|
||||
`R13` at 24 rows and `R40` at 60 — one script, two documents. It became live
|
||||
when #151 let a script's commands see the cursor (improvise-6uva); before that
|
||||
the cursor was always the origin and every run hid `R01`.
|
||||
|
||||
The test runs the same script against two copies of one document at `24x80`
|
||||
and `60x200` and asserts the same record is hidden. One size cannot see the
|
||||
bug, which is the ticket's own requirement. The size is driven through `LINES`
|
||||
and `COLUMNS` with the child detached from any terminal by `setsid`: crossterm
|
||||
asks `/dev/tty` first and falls back to `tput`, which honours those variables,
|
||||
so without the detach a developer's real terminal would override both sizes and
|
||||
the test would pass against the bug on a laptop and fail it only in CI.
|
||||
|
||||
Two guards against vacuity. The hidden record must not be `R01`, because a
|
||||
cursor that never moved agrees at every size (the pre-#151 shape, and the shape
|
||||
of a fix that re-broke 6uva). And something must be hidden at all. Failed at
|
||||
`9e78051` with `R13` against `R40`. It cannot see a *width* leak, and does not
|
||||
need to: width moves only `col_offset`, which no command today turns into a
|
||||
written record.
|
||||
|
||||
### a_commit_whose_advance_has_nowhere_to_go_still_exits_zero
|
||||
|
||||
improvise-c5ip. `commit-cell-edit` ends in `EnterAdvance`, which emits
|
||||
`AbortChain` when the cursor is already bottom-right; the CLI mapped
|
||||
`last_batch_aborted()` to exit 1, so a write that landed **and was saved**
|
||||
exited 1 with nothing on stderr. Two cases: the cursor moved to the last of
|
||||
three rows and the write named explicitly, and a commit on a grid that is empty
|
||||
until the same line creates it. The value is asserted **first**, so the test
|
||||
cannot pass by the write simply not happening. Failed at `487f0b6` on the exit
|
||||
assertion with `South = 9` already written. Rival: keeping
|
||||
`|| last_batch_aborted()` in `main.rs` beside the new refusal check — fails
|
||||
here, at the first exit assertion.
|
||||
|
||||
### a_refusal_that_only_writes_a_status_exits_non_zero
|
||||
|
||||
improvise-udt2. `add-item Nonexistent Thing` writes the workbook's error to the
|
||||
status and does not abort, so it exited 0. It asserts the refusal was printed
|
||||
before asserting the exit, so it cannot pass on a run that failed for some other
|
||||
reason. Rival: `AddItem` writing `status_msg` directly instead of calling
|
||||
`App::refuse` — fails here. The two rivals the ticket named are the existing
|
||||
`a_command_that_succeeds_while_reporting_still_exits_zero` (a successful `:w`
|
||||
calling `refuse` fails it, measured) and `a_fit_that_reports_its_seed_is_not_a_refusal`.
|
||||
|
||||
### a_refusal_marks_only_its_own_batch
|
||||
|
||||
A refusing batch followed by a reporting one. The second assertion is the one
|
||||
the default cannot supply: a flag that is never reset passes the first half and
|
||||
fails the second. Rival: deleting the reset in `apply_effects` — fails at the
|
||||
second assertion. Rival: `AddItem` not calling `refuse` — fails at the first.
|
||||
|
||||
### a_chain_with_nowhere_to_go_aborts_without_refusing
|
||||
|
||||
`AbortChain` must abort — asserted, so the test cannot pass on a batch that did
|
||||
nothing — and must not refuse. Rival: `AbortChain` calling `refuse` — fails
|
||||
here, and fails the process-level c5ip test too.
|
||||
|
||||
### a_failed_save_is_a_refusal_as_well_as_an_abort
|
||||
|
||||
The one place the two flags coincide, and the reason the old exit code looked
|
||||
right: a failed file write both stops the chain (gotcha 21) and refuses. It
|
||||
keeps `an_aborted_batch_exits_non_zero_and_says_why` meaningful now that the CLI
|
||||
reads only the refusal.
|
||||
|
||||
### a_fit_that_reports_its_seed_is_not_a_refusal
|
||||
|
||||
udt2's second rival: `fit-measure` speaks on **success**, carrying the seed.
|
||||
It asserts the status mentions the seed first, so a fit that said nothing
|
||||
cannot pass. Rival: the success arm emitting `refuse` instead of `set_status` —
|
||||
fails here. The refusal arm is asserted in
|
||||
`a_refusal_names_what_went_wrong_and_stores_nothing`.
|
||||
|
||||
@@ -4389,7 +4389,22 @@ producer (fails with `None`).
|
||||
Records mode is deliberately not covered. There the TUI yanks `None` and its
|
||||
`p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key
|
||||
(improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep
|
||||
case there would assert the bug.
|
||||
case there would assert the bug. Since improvise-6p8q's fix it is covered, by
|
||||
`yy_then_p_in_records_mode_writes_the_record_the_host_writes` below.
|
||||
|
||||
### yy_then_p_in_records_mode_writes_the_record_the_host_writes
|
||||
|
||||
improvise-6p8q in lockstep. The yank test in this file deliberately stayed on a
|
||||
pivot grid because a records case would have asserted the TUI bug. Two records
|
||||
(North, South), the yank from North and the paste onto South on row 1, the
|
||||
`Revenue` column being neither the first column nor the last. Rivals, each run:
|
||||
restoring the synthetic-key paste fails `both_ends`' document comparison on
|
||||
`p` (the twin applied the client's `SetCell` to South's record, the host wrote
|
||||
a `_Dim`/`_Index` cell); restoring the synthetic-key yank fails the yank
|
||||
assertion. The tail yanks off the `Region` and `+` columns and requires the two
|
||||
registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on
|
||||
`+`, which is why that filter was removed rather than kept. The evidence entries
|
||||
for the host-side tests are in `command.md`.
|
||||
|
||||
## Numbers across the serialized wire (improvise-6oy4)
|
||||
|
||||
|
||||
@@ -5044,3 +5044,69 @@ bracket-matched `#[…]` before reading the name.
|
||||
`an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture:
|
||||
a doc comment, two attributes (one with nested brackets), and a wrapped call.
|
||||
With the whitespace skip removed it fails with `[]`.
|
||||
|
||||
### yy_then_p_in_records_mode_copies_between_real_records
|
||||
|
||||
improvise-6p8q. At `gh/main` `9e78051` it failed at the yank: `left: None,
|
||||
right: Some(Number(700.0))`, because `YankCell` evaluated the synthetic
|
||||
`(_Index, _Dim)` key. With the yank alone repaired (rival: paste still writes
|
||||
`SetCell(synthetic key)`), it fails at the paste assertion, and the document
|
||||
carried `_Dim=|3|, _Index=|0| = …` in `## Data`. The test drives `yy`/`p`
|
||||
through `App::handle_key` and then saves and reloads, because the persisted
|
||||
line is the harm. The fixture is chosen so no two answers coincide: two records
|
||||
by two measures with four distinct values, the yank from row 0 `Cost` and the
|
||||
paste onto row 1 `Revenue`, so "the first row", "the first measure" and "the
|
||||
value already there" are each wrong. The persistence assertion is scoped to the
|
||||
`## Data` section: a view section legitimately writes `_Index: none`, and a
|
||||
first version of this test failed on that.
|
||||
|
||||
### yy_on_a_records_coordinate_column_yanks_the_item_as_text
|
||||
|
||||
A coordinate column yanks its item as text (`West`, row 1): the yank reads
|
||||
`GridLayout::records_value`, the same value the browser yanks off
|
||||
`CellDisplay::value`. Rival: restoring the synthetic-key yank fails it with
|
||||
`None`.
|
||||
|
||||
### p_on_a_records_column_that_stores_no_value_changes_nothing
|
||||
|
||||
`p` on `Region`, `+cat` and `+` at row 1 leaves the document byte-identical and
|
||||
says why. At `9e78051` it failed on `Region` with `_Dim=|0|, _Index=|1| = 5.5`
|
||||
appended to `## Data`. The control — the same paste onto `Revenue` writes — is
|
||||
what keeps the declines from being "this command never writes".
|
||||
|
||||
### p_inside_a_drill_neither_writes_nor_stages
|
||||
|
||||
At `9e78051` a drilled paste wrote `_Dim=|3|, _Index=|0| = 5.5`. The rival that
|
||||
resolves the real record but ignores the drill (writing straight through) fails
|
||||
the document-unchanged assertion at the same line. Why declining rather than
|
||||
staging: `context/code/command.md`, "Paste lands on a record or nowhere".
|
||||
|
||||
### p_onto_a_none_axis_aggregate_writes_nothing
|
||||
|
||||
improvise-l4mp, None-axis half. At `9e78051` `p` over the West/Revenue aggregate
|
||||
(Product on None) wrote `Region=West, _Measure=Revenue = 5.5` — a partial key
|
||||
that is not a record. The cursor is on row 1, not row 0. The control reruns the
|
||||
same paste with `Product` paged instead of aggregated and requires the write,
|
||||
so a paste that declines everywhere fails. Rival: removing the aggregate check
|
||||
fails the first assertion.
|
||||
|
||||
### paste_with_an_explicit_full_key_writes_even_where_the_view_aggregates
|
||||
|
||||
Pins that "aggregate" is asked of the key, not of the view. It passes at
|
||||
`9e78051` (there was no check at all); its job is the rival where the check
|
||||
reads only whether the view has a regular category on None, which fails it —
|
||||
that rival would silently turn every scripted `paste` with full coordinates
|
||||
into a no-op on an aggregating view.
|
||||
|
||||
### the_aggregate_decline_does_not_send_the_user_where_paste_also_declines
|
||||
|
||||
improvise-48jd, from the #184 review. The aggregate decline said "drill in to
|
||||
paste into a record", and inside the drill `p` answers `PASTE_INSIDE_DRILL`, so
|
||||
the advice led to a second refusal. The test follows the advice literally: `p`
|
||||
on the West aggregate, `>` into its drill, cursor onto the `Revenue` column, `p`
|
||||
again. It asserts that the drilled paste really is the decline, so the setup
|
||||
cannot pass by landing on "no cell at cursor". A first version of the test did
|
||||
exactly that, because West had no record to drill into. It then asserts that
|
||||
the first message does not mention drilling. At `0ed2e41` it failed with the
|
||||
old text. The rival is restoring that text. Real drill paste, which means
|
||||
staging a typed `CellValue`, is still open as improvise-48jd.
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
use crate::model::Aggregation;
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::ui::effect::{self, Effect, ViewTarget};
|
||||
use crate::view::synthetic_record_info;
|
||||
|
||||
use super::core::{Cmd, CmdContext};
|
||||
|
||||
@@ -91,6 +93,276 @@ mod tests {
|
||||
assert!(effects.is_empty());
|
||||
}
|
||||
|
||||
use crate::command::key::{KeyCode, KeyEvent, KeyModifiers};
|
||||
use crate::ui::app::App;
|
||||
use crate::workbook::Workbook;
|
||||
|
||||
fn press(app: &mut App, c: char) {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
fn record(region: &str, measure: &str) -> CellKey {
|
||||
CellKey::new(vec![
|
||||
("Region".into(), region.into()),
|
||||
("_Measure".into(), measure.into()),
|
||||
])
|
||||
}
|
||||
|
||||
/// Two records by two measures, every value distinct, in records mode.
|
||||
fn records_app() -> App {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_item("Region", "East").unwrap();
|
||||
wb.add_item("Region", "West").unwrap();
|
||||
wb.add_item("_Measure", "Revenue").unwrap();
|
||||
wb.add_item("_Measure", "Cost").unwrap();
|
||||
for (region, measure, n) in [
|
||||
("East", "Revenue", 1200.0),
|
||||
("East", "Cost", 700.0),
|
||||
("West", "Revenue", 900.0),
|
||||
("West", "Cost", 400.0),
|
||||
] {
|
||||
wb.model
|
||||
.set_cell(record(region, measure), CellValue::Number(n));
|
||||
}
|
||||
let mut app = App::new(wb, None);
|
||||
press(&mut app, 'R');
|
||||
assert!(app.layout.is_records_mode(), "setup: in records mode");
|
||||
app
|
||||
}
|
||||
|
||||
fn col_of(app: &App, label: &str) -> usize {
|
||||
(0..app.layout.col_count())
|
||||
.find(|&c| app.layout.col_label(c) == label)
|
||||
.unwrap_or_else(|| panic!("no records column {label}"))
|
||||
}
|
||||
|
||||
fn synthetic_cells(wb: &Workbook) -> Vec<CellKey> {
|
||||
wb.model
|
||||
.data
|
||||
.iter_cells()
|
||||
.map(|(k, _)| k)
|
||||
.filter(|k| k.get("_Index").is_some() || k.get("_Dim").is_some())
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn yy_then_p_in_records_mode_copies_between_real_records() {
|
||||
let mut app = records_app();
|
||||
let (east, west) = (0, 1);
|
||||
assert_eq!(
|
||||
app.layout.record_row(west).unwrap().coords.get("Region"),
|
||||
Some("West")
|
||||
);
|
||||
|
||||
app.cursor_mut().selected = (east, col_of(&app, "Cost"));
|
||||
press(&mut app, 'y');
|
||||
press(&mut app, 'y');
|
||||
assert_eq!(
|
||||
app.view_state.yanked,
|
||||
Some(CellValue::Number(700.0)),
|
||||
"yy must yank the record's stored value, not the synthetic key's"
|
||||
);
|
||||
|
||||
app.cursor_mut().selected = (west, col_of(&app, "Revenue"));
|
||||
press(&mut app, 'p');
|
||||
let wb = &app.model_state.workbook;
|
||||
assert_eq!(
|
||||
wb.model.get_cell(&record("West", "Revenue")),
|
||||
Some(&CellValue::Number(700.0)),
|
||||
"p must write the real record under the cursor"
|
||||
);
|
||||
assert_eq!(
|
||||
wb.model.get_cell(&record("East", "Cost")),
|
||||
Some(&CellValue::Number(700.0))
|
||||
);
|
||||
assert_eq!(synthetic_cells(wb), Vec::<CellKey>::new());
|
||||
|
||||
let text = crate::persistence::format_md(wb);
|
||||
let data = text.split("## Data").nth(1).expect("a data section");
|
||||
assert!(
|
||||
!data.contains("_Index") && !data.contains("_Dim"),
|
||||
"the saved document carries a synthetic-keyed line:\n{text}"
|
||||
);
|
||||
let reloaded = crate::persistence::parse_md(&text).expect("round trip");
|
||||
assert_eq!(
|
||||
reloaded.model.get_cell(&record("West", "Revenue")),
|
||||
Some(&CellValue::Number(700.0))
|
||||
);
|
||||
assert_eq!(synthetic_cells(&reloaded), Vec::<CellKey>::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn yy_on_a_records_coordinate_column_yanks_the_item_as_text() {
|
||||
let mut app = records_app();
|
||||
app.cursor_mut().selected = (1, col_of(&app, "Region"));
|
||||
press(&mut app, 'y');
|
||||
press(&mut app, 'y');
|
||||
assert_eq!(app.view_state.yanked, Some(CellValue::Text("West".into())));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn p_on_a_records_column_that_stores_no_value_changes_nothing() {
|
||||
for label in ["Region", "+cat", "+"] {
|
||||
let mut app = records_app();
|
||||
app.view_state.yanked = Some(CellValue::Number(5.5));
|
||||
let before = crate::persistence::format_md(&app.model_state.workbook);
|
||||
app.cursor_mut().selected = (1, col_of(&app, label));
|
||||
press(&mut app, 'p');
|
||||
assert_eq!(
|
||||
crate::persistence::format_md(&app.model_state.workbook),
|
||||
before,
|
||||
"p on the {label} column changed the document"
|
||||
);
|
||||
assert!(
|
||||
!app.view_state.status_msg.is_empty(),
|
||||
"p on the {label} column declined silently"
|
||||
);
|
||||
}
|
||||
// The control: the same paste onto a measure column does write.
|
||||
let mut app = records_app();
|
||||
app.view_state.yanked = Some(CellValue::Number(5.5));
|
||||
app.cursor_mut().selected = (1, col_of(&app, "Revenue"));
|
||||
press(&mut app, 'p');
|
||||
assert_eq!(
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.get_cell(&record("West", "Revenue")),
|
||||
Some(&CellValue::Number(5.5)),
|
||||
"the control never wrote, so the declines above prove nothing"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn p_inside_a_drill_neither_writes_nor_stages() {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_item("Region", "East").unwrap();
|
||||
wb.add_item("_Measure", "Revenue").unwrap();
|
||||
wb.add_item("_Measure", "Cost").unwrap();
|
||||
wb.model
|
||||
.set_cell(record("East", "Revenue"), CellValue::Number(1.0));
|
||||
wb.model
|
||||
.set_cell(record("East", "Cost"), CellValue::Number(2.0));
|
||||
let mut app = App::new(wb, None);
|
||||
press(&mut app, '>');
|
||||
assert!(app.drill_state.is_some(), "setup: drilled");
|
||||
let before = crate::persistence::format_md(&app.model_state.workbook);
|
||||
app.view_state.yanked = Some(CellValue::Number(5.5));
|
||||
app.cursor_mut().selected = (0, col_of(&app, "Cost"));
|
||||
press(&mut app, 'p');
|
||||
assert_eq!(
|
||||
crate::persistence::format_md(&app.model_state.workbook),
|
||||
before
|
||||
);
|
||||
assert!(
|
||||
app.drill_state.as_ref().unwrap().pending_edits.is_empty(),
|
||||
"a paste was staged as text, which loses the value's type"
|
||||
);
|
||||
assert_eq!(
|
||||
synthetic_cells(&app.model_state.workbook),
|
||||
Vec::<CellKey>::new()
|
||||
);
|
||||
assert!(!app.view_state.status_msg.is_empty(), "declined silently");
|
||||
}
|
||||
|
||||
/// Region on rows, Product aggregated away on None: every cell is an aggregate.
|
||||
fn aggregated_app(product_axis: crate::view::Axis) -> App {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_item("Region", "East").unwrap();
|
||||
wb.add_item("Region", "West").unwrap();
|
||||
wb.add_category("Product").unwrap();
|
||||
wb.add_item("Product", "Widget").unwrap();
|
||||
wb.add_item("Product", "Gadget").unwrap();
|
||||
wb.add_item("_Measure", "Revenue").unwrap();
|
||||
wb.active_view_mut()
|
||||
.set_axis("Region", crate::view::Axis::Row);
|
||||
wb.active_view_mut()
|
||||
.set_axis("_Measure", crate::view::Axis::Column);
|
||||
wb.active_view_mut().set_axis("Product", product_axis);
|
||||
wb.model.set_cell(
|
||||
record("East", "Revenue").with("Product", "Widget"),
|
||||
CellValue::Number(10.0),
|
||||
);
|
||||
let mut app = App::new(wb, None);
|
||||
app.view_state.yanked = Some(CellValue::Number(5.5));
|
||||
app.cursor_mut().selected = (1, 0);
|
||||
app
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn p_onto_a_none_axis_aggregate_writes_nothing() {
|
||||
let mut app = aggregated_app(crate::view::Axis::None);
|
||||
let before = crate::persistence::format_md(&app.model_state.workbook);
|
||||
press(&mut app, 'p');
|
||||
assert_eq!(
|
||||
crate::persistence::format_md(&app.model_state.workbook),
|
||||
before,
|
||||
"p wrote a cell at the aggregate's partial key"
|
||||
);
|
||||
assert!(!app.view_state.status_msg.is_empty(), "declined silently");
|
||||
|
||||
// The control: with Product paged rather than aggregated, p writes.
|
||||
let mut app = aggregated_app(crate::view::Axis::Page);
|
||||
press(&mut app, 'p');
|
||||
assert_eq!(
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.get_cell(&record("West", "Revenue").with("Product", "Widget")),
|
||||
Some(&CellValue::Number(5.5))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_aggregate_decline_does_not_send_the_user_where_paste_also_declines() {
|
||||
let mut app = aggregated_app(crate::view::Axis::None);
|
||||
app.model_state.workbook.model.set_cell(
|
||||
record("West", "Revenue").with("Product", "Gadget"),
|
||||
CellValue::Number(3.0),
|
||||
);
|
||||
app.rebuild_layout();
|
||||
press(&mut app, 'p');
|
||||
let advice = app.view_state.status_msg.clone();
|
||||
press(&mut app, '>');
|
||||
assert!(app.drill_state.is_some(), "setup: the aggregate drills");
|
||||
app.cursor_mut().selected = (0, col_of(&app, "Revenue"));
|
||||
press(&mut app, 'p');
|
||||
let in_drill = app.view_state.status_msg.clone();
|
||||
assert_eq!(
|
||||
in_drill, PASTE_INSIDE_DRILL,
|
||||
"setup: the drilled paste declines"
|
||||
);
|
||||
assert!(
|
||||
!advice.to_lowercase().contains("drill"),
|
||||
"{advice:?} sends the user into a drill, where p answers {in_drill:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn paste_with_an_explicit_full_key_writes_even_where_the_view_aggregates() {
|
||||
let app = aggregated_app(crate::view::Axis::None);
|
||||
let wb = &app.model_state.workbook;
|
||||
let layout = make_layout(wb);
|
||||
let reg = make_registry();
|
||||
let yanked = Some(CellValue::Number(5.5));
|
||||
let mut ctx = make_ctx(wb, &layout, ®);
|
||||
ctx.yanked = &yanked;
|
||||
assert!(!ctx.none_cats().is_empty(), "setup: the view aggregates");
|
||||
let key = record("West", "Revenue").with("Product", "Gadget");
|
||||
let effects = PasteCell { key: key.clone() }.execute(&ctx);
|
||||
let mut app = aggregated_app(crate::view::Axis::None);
|
||||
app.apply_effects(effects);
|
||||
assert_eq!(
|
||||
app.model_state.workbook.model.get_cell(&key),
|
||||
Some(&CellValue::Number(5.5)),
|
||||
"a key naming every coordinate is a record, whatever the view shows"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn transpose_produces_transpose() {
|
||||
let m = two_cat_model();
|
||||
@@ -146,9 +418,13 @@ impl Cmd for YankCell {
|
||||
"yank"
|
||||
}
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
let value = ctx
|
||||
.model
|
||||
.evaluate_aggregated(&self.key, Aggregation::of(ctx.none_cats()));
|
||||
// A records cell is the layout's one resolution, as the browser yanks it (gotcha 1).
|
||||
let value = match synthetic_record_info(&self.key) {
|
||||
Some((row, col)) => ctx.layout.records_value(row, col),
|
||||
None => ctx
|
||||
.model
|
||||
.evaluate_aggregated(&self.key, Aggregation::of(ctx.none_cats())),
|
||||
};
|
||||
vec![
|
||||
Box::new(effect::SetYanked(value)),
|
||||
effect::set_status("Yanked"),
|
||||
@@ -166,14 +442,47 @@ impl Cmd for PasteCell {
|
||||
"paste"
|
||||
}
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
if let Some(value) = ctx.yanked.clone() {
|
||||
vec![Box::new(effect::SetCell(self.key.clone(), value))]
|
||||
} else {
|
||||
vec![]
|
||||
match ctx.yanked.clone() {
|
||||
Some(value) => vec![paste_into(ctx, &self.key, value)],
|
||||
None => vec![],
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Where a paste at `key` lands. The reasoning, and why a drill declines:
|
||||
/// `context/code/command.md`, "Paste lands on a record or nowhere".
|
||||
fn paste_into(ctx: &CmdContext, key: &CellKey, value: CellValue) -> Box<dyn Effect> {
|
||||
match synthetic_record_info(key) {
|
||||
Some((row, col)) => paste_into_record(ctx, row, col, value),
|
||||
None if aggregates(ctx, key) => effect::set_status(PASTE_ONTO_AGGREGATE),
|
||||
None => Box::new(effect::SetCell(key.clone(), value)),
|
||||
}
|
||||
}
|
||||
|
||||
fn paste_into_record(
|
||||
ctx: &CmdContext,
|
||||
row: usize,
|
||||
col: usize,
|
||||
value: CellValue,
|
||||
) -> Box<dyn Effect> {
|
||||
match (ctx.layout.record_cell_key(row, col), ctx.has_drill_state) {
|
||||
(Some(_), true) => effect::set_status(PASTE_INSIDE_DRILL),
|
||||
(Some(real), false) => Box::new(effect::SetCell(real, value)),
|
||||
(None, _) => effect::set_status(PASTE_ONTO_NO_VALUE),
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether `key` leaves out a regular category the layout aggregates over.
|
||||
fn aggregates(ctx: &CmdContext, key: &CellKey) -> bool {
|
||||
ctx.none_cats().iter().any(|cat| {
|
||||
key.get(cat).is_none() && ctx.model.category(cat).is_some_and(|c| c.kind.is_regular())
|
||||
})
|
||||
}
|
||||
|
||||
const PASTE_ONTO_AGGREGATE: &str = "Cannot paste onto an aggregate — it combines several records, so there is no one cell to write";
|
||||
const PASTE_ONTO_NO_VALUE: &str = "Paste lands only on a measure column";
|
||||
const PASTE_INSIDE_DRILL: &str = "Paste is not staged inside a drill — edit the cell instead";
|
||||
|
||||
// ── View commands ────────────────────────────────────────────────────────────
|
||||
|
||||
#[derive(Debug)]
|
||||
|
||||
@@ -343,7 +343,7 @@ mod tests {
|
||||
!dbg.contains("SaveAs"),
|
||||
"an empty path must not reach SaveAs, got: {dbg}"
|
||||
);
|
||||
assert!(dbg.contains("SetStatus"), "Expected a status, got: {dbg}");
|
||||
assert!(dbg.contains("Refuse"), "Expected a refusal, got: {dbg}");
|
||||
}
|
||||
|
||||
/// A one-record records grid with `Region` as its only category and `Revenue` as
|
||||
@@ -507,7 +507,7 @@ fn rename_record_coordinate(
|
||||
effects: &mut Vec<Box<dyn Effect>>,
|
||||
) {
|
||||
if value.is_empty() {
|
||||
effects.push(effect::set_status(effect::RECORD_COORDS_CANNOT_BE_EMPTY));
|
||||
effects.push(effect::refuse(effect::RECORD_COORDS_CANNOT_BE_EMPTY));
|
||||
return;
|
||||
}
|
||||
effects.push(Box::new(effect::AddItem {
|
||||
@@ -836,7 +836,7 @@ impl Cmd for CommitSave {
|
||||
let path = read_buffer(ctx, "save").trim().to_string();
|
||||
if path.is_empty() {
|
||||
return vec![
|
||||
effect::set_status("No path given — not saved"),
|
||||
effect::refuse("No path given — not saved"),
|
||||
effect::change_mode(AppMode::Normal),
|
||||
];
|
||||
}
|
||||
@@ -860,7 +860,7 @@ impl Cmd for CommitExport {
|
||||
let path = read_buffer(ctx, "export").trim().to_string();
|
||||
if path.is_empty() {
|
||||
return vec![
|
||||
effect::set_status("No path given — not exported"),
|
||||
effect::refuse("No path given — not exported"),
|
||||
effect::change_mode(AppMode::Normal),
|
||||
];
|
||||
}
|
||||
|
||||
@@ -83,7 +83,7 @@ impl Cmd for FitMeasure {
|
||||
],
|
||||
// Flattened: a refusal interpolates a document name, and a name may
|
||||
// legally contain a newline (Copilot on PR #25).
|
||||
Err(e) => vec![effect::set_status(one_line(format!("fit-measure: {e}")))],
|
||||
Err(e) => vec![effect::refuse(one_line(format!("fit-measure: {e}")))],
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -221,6 +221,19 @@ mod tests {
|
||||
app.view_state.status_msg
|
||||
);
|
||||
assert!(app.model_state.workbook.model.formulas().is_empty());
|
||||
assert!(app.last_batch_refused(), "a refused fit exits non-zero");
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/cli.md`.
|
||||
#[test]
|
||||
fn a_fit_that_reports_its_seed_is_not_a_refusal() {
|
||||
let app = run(&["Score"]);
|
||||
assert!(
|
||||
app.view_state.status_msg.contains("seed"),
|
||||
"the fit said nothing, so this proves nothing: {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
assert!(!app.last_batch_refused());
|
||||
}
|
||||
|
||||
/// gotcha 37b: a different seed really does search differently. It asserts on the
|
||||
|
||||
@@ -546,13 +546,13 @@ impl Cmd for ViewNavigate {
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
if self.forward {
|
||||
if ctx.view_forward_stack.is_empty() {
|
||||
vec![effect::set_status("No forward view")]
|
||||
vec![effect::refuse("No forward view")]
|
||||
} else {
|
||||
vec![Box::new(effect::ViewForward)]
|
||||
}
|
||||
} else {
|
||||
if ctx.view_back_stack.is_empty() {
|
||||
vec![effect::set_status("No previous view")]
|
||||
vec![effect::refuse("No previous view")]
|
||||
} else {
|
||||
vec![
|
||||
Box::new(effect::ApplyAndClearDrill),
|
||||
@@ -693,9 +693,7 @@ impl Cmd for AddRecordRow {
|
||||
}
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
if !ctx.is_records_mode() {
|
||||
return vec![effect::set_status(
|
||||
"add-record-row only works in records mode",
|
||||
)];
|
||||
return vec![effect::refuse("add-record-row only works in records mode")];
|
||||
}
|
||||
// Build a CellKey from the current page filters
|
||||
let view = ctx.view;
|
||||
@@ -723,7 +721,7 @@ impl Cmd for AddRecordRow {
|
||||
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
|
||||
match existing {
|
||||
None => effects.push(Box::new(effect::SetCell(key, CellValue::Number(0.0)))),
|
||||
Some(_) => effects.push(effect::set_status(
|
||||
Some(_) => effects.push(effect::refuse(
|
||||
"That record row is already here — a row is its coordinates, so \
|
||||
fill this one in first",
|
||||
)),
|
||||
|
||||
@@ -373,14 +373,14 @@ impl Cmd for EditOrDrill {
|
||||
let is_aggregated = !is_synthetic && regular_none;
|
||||
if is_aggregated {
|
||||
let Some(key) = ctx.cell_key().clone() else {
|
||||
return vec![effect::set_status(self.nothing_at_cursor())];
|
||||
return vec![effect::refuse(self.nothing_at_cursor())];
|
||||
};
|
||||
return DrillIntoCell { key }.execute(ctx);
|
||||
}
|
||||
// The same question `cell_id` asks, off the same predicate. In records mode
|
||||
// the synthetic key names no measure, so the record's key is resolved.
|
||||
if let Some(formula) = ctx.owning_formula() {
|
||||
return vec![effect::set_status(format!(
|
||||
return vec![effect::refuse(format!(
|
||||
"{} is computed by a formula — edit the formula, not the cell",
|
||||
formula.target
|
||||
))];
|
||||
|
||||
@@ -127,7 +127,7 @@ impl Cmd for SearchNavigate {
|
||||
.collect();
|
||||
|
||||
if matches.is_empty() {
|
||||
return vec![effect::set_status(format!(
|
||||
return vec![effect::refuse(format!(
|
||||
"No matches for '{}'",
|
||||
ctx.search_query
|
||||
))];
|
||||
|
||||
@@ -379,7 +379,7 @@ impl Cmd for ExecuteCommand {
|
||||
}
|
||||
Err(msg) => {
|
||||
vec![
|
||||
effect::set_status(format!(":{raw} — {msg}")),
|
||||
effect::refuse(format!(":{raw} — {msg}")),
|
||||
effect::change_mode(AppMode::Normal),
|
||||
]
|
||||
}
|
||||
|
||||
@@ -314,6 +314,9 @@ pub struct App {
|
||||
/// Whether the **last** `apply_effects` batch stopped early. Complete only
|
||||
/// for a one-effect batch — see `context/code/command.md`.
|
||||
batch_aborted: bool,
|
||||
/// Whether an effect in the **last** batch refused what it was asked, through
|
||||
/// [`App::refuse`]. Not an abort: `context/code/cli.md` §10.
|
||||
batch_refused: bool,
|
||||
/// **Session-local**: how many times the whole document has been replaced.
|
||||
/// Bumped by `install_workbook` and nothing else (improvise-cqq).
|
||||
document_generation: u64,
|
||||
@@ -355,6 +358,7 @@ pub struct AppSnapshot {
|
||||
view_state: ViewState,
|
||||
abort_effects: bool,
|
||||
batch_aborted: bool,
|
||||
batch_refused: bool,
|
||||
last_reshaped_views: std::collections::BTreeSet<String>,
|
||||
last_reframed_views: std::collections::BTreeSet<String>,
|
||||
written_view_frames: std::collections::BTreeMap<String, u64>,
|
||||
@@ -402,6 +406,7 @@ impl App {
|
||||
layout,
|
||||
abort_effects: false,
|
||||
batch_aborted: false,
|
||||
batch_refused: false,
|
||||
document_generation: 0,
|
||||
layout_version: 0,
|
||||
cursor_epoch: 0,
|
||||
@@ -451,6 +456,19 @@ impl App {
|
||||
self.batch_aborted
|
||||
}
|
||||
|
||||
/// Did an effect in the last batch refuse what it was asked? The question an
|
||||
/// exit code asks, which `last_batch_aborted` does not answer (improvise-c5ip).
|
||||
pub fn last_batch_refused(&self) -> bool {
|
||||
self.batch_refused
|
||||
}
|
||||
|
||||
/// Say why the requested change did not happen, and record that it did not.
|
||||
/// The one writer of `batch_refused`, so a refusal cannot forget the flag.
|
||||
pub fn refuse(&mut self, msg: impl Into<String>) {
|
||||
self.view_state.status_msg = msg.into();
|
||||
self.batch_refused = true;
|
||||
}
|
||||
|
||||
/// 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(&self) -> AppSnapshot {
|
||||
@@ -463,6 +481,7 @@ impl App {
|
||||
view_state: self.view_state.clone(),
|
||||
abort_effects: self.abort_effects,
|
||||
batch_aborted: self.batch_aborted,
|
||||
batch_refused: self.batch_refused,
|
||||
last_reshaped_views: self.last_reshaped_views.clone(),
|
||||
last_reframed_views: self.last_reframed_views.clone(),
|
||||
written_view_frames: self.written_view_frames.clone(),
|
||||
@@ -481,6 +500,7 @@ impl App {
|
||||
self.view_state = snapshot.view_state;
|
||||
self.abort_effects = snapshot.abort_effects;
|
||||
self.batch_aborted = snapshot.batch_aborted;
|
||||
self.batch_refused = snapshot.batch_refused;
|
||||
self.last_reshaped_views = snapshot.last_reshaped_views;
|
||||
self.last_reframed_views = snapshot.last_reframed_views;
|
||||
self.written_view_frames = snapshot.written_view_frames;
|
||||
@@ -818,6 +838,7 @@ impl App {
|
||||
pub fn apply_effects(&mut self, effects: Vec<Box<dyn super::effect::Effect>>) -> Dirty {
|
||||
self.abort_effects = false;
|
||||
self.batch_aborted = false;
|
||||
self.batch_refused = false;
|
||||
// The `Dirty` is read off the `EffectKind`; the two lower basis tiers are
|
||||
// **observed** around the loop (improvise-75ld, improvise-pdvm).
|
||||
let touches_model = effects
|
||||
|
||||
@@ -83,7 +83,7 @@ impl Effect for AddCategory {
|
||||
match app.model_state.workbook.add_category(&self.0) {
|
||||
Ok(_) => {}
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = e.to_string();
|
||||
app.refuse(e.to_string());
|
||||
app.abort_effects = true;
|
||||
}
|
||||
}
|
||||
@@ -105,7 +105,7 @@ impl Effect for AddItem {
|
||||
.workbook
|
||||
.add_item(&self.category, &self.item)
|
||||
{
|
||||
app.view_state.status_msg = e.to_string();
|
||||
app.refuse(e.to_string());
|
||||
}
|
||||
}
|
||||
fn kind(&self) -> EffectKind {
|
||||
@@ -126,7 +126,7 @@ impl Effect for AddItemInGroup {
|
||||
.workbook
|
||||
.add_item_in_group(&self.category, &self.item, &self.group)
|
||||
{
|
||||
app.view_state.status_msg = e.to_string();
|
||||
app.refuse(e.to_string());
|
||||
}
|
||||
}
|
||||
fn kind(&self) -> EffectKind {
|
||||
@@ -192,7 +192,7 @@ impl Effect for AddFormula {
|
||||
app.model_state.workbook.model.add_formula(formula);
|
||||
}
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("Formula error: {e}");
|
||||
app.refuse(format!("Formula error: {e}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -558,6 +558,19 @@ impl Effect for ReturnToNormalUnlessMoved {
|
||||
|
||||
simple_effect!(SetStatus(String) => |app| app.view_state.status_msg, EffectKind::ViewOnly);
|
||||
|
||||
/// A command's own refusal: the status line, plus the record that nothing it was
|
||||
/// asked for happened. [`SetStatus`] is for reports — `context/code/cli.md` §10.
|
||||
#[derive(Debug)]
|
||||
pub struct Refuse(pub String);
|
||||
impl Effect for Refuse {
|
||||
fn apply(&self, app: &mut App) {
|
||||
app.refuse(self.0.clone());
|
||||
}
|
||||
fn kind(&self) -> EffectKind {
|
||||
EffectKind::ViewOnly
|
||||
}
|
||||
}
|
||||
|
||||
simple_effect!(SetYanked(Option<CellValue>) => |app| app.view_state.yanked, EffectKind::ViewOnly);
|
||||
|
||||
simple_effect!(SetSearchQuery(String) => |app| app.view_state.search_query, EffectKind::ViewOnly);
|
||||
@@ -656,7 +669,7 @@ fn apply_measure_edit(app: &mut App, key: &CellKey, new_value: &str) {
|
||||
/// 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.view_state.status_msg = RECORD_COORDS_CANNOT_BE_EMPTY.to_string();
|
||||
app.refuse(RECORD_COORDS_CANNOT_BE_EMPTY);
|
||||
return;
|
||||
}
|
||||
let keys: Vec<CellKey> = if row.values.is_empty() {
|
||||
@@ -726,7 +739,7 @@ fn report_file_write(app: &mut App, outcome: Result<String, String>) {
|
||||
match outcome {
|
||||
Ok(msg) => app.view_state.status_msg = msg,
|
||||
Err(msg) => {
|
||||
app.view_state.status_msg = msg;
|
||||
app.refuse(msg);
|
||||
app.abort_effects = true;
|
||||
}
|
||||
}
|
||||
@@ -878,11 +891,11 @@ impl Effect for StartImportWizard {
|
||||
app.view_state.mode = AppMode::ImportWizard;
|
||||
}
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("JSON parse error: {e}");
|
||||
app.refuse(format!("JSON parse error: {e}"));
|
||||
}
|
||||
},
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("Cannot read file: {e}");
|
||||
app.refuse(format!("Cannot read file: {e}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -922,7 +935,7 @@ impl Effect for LoadModel {
|
||||
app.view_state.status_msg = format!("Loaded from {}", self.0.display());
|
||||
}
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("Load error: {e}");
|
||||
app.refuse(format!("Load error: {e}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -980,8 +993,7 @@ impl Effect for ImportJsonHeadless {
|
||||
let bytes = match app.storage.read(&self.path) {
|
||||
Ok(b) => b,
|
||||
Err(e) => {
|
||||
app.view_state.status_msg =
|
||||
format!("Cannot read '{}': {e}", self.path.display());
|
||||
app.refuse(format!("Cannot read '{}': {e}", self.path.display()));
|
||||
return;
|
||||
}
|
||||
};
|
||||
@@ -992,7 +1004,7 @@ impl Effect for ImportJsonHeadless {
|
||||
(recs, raw)
|
||||
}
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("CSV error: {e}");
|
||||
app.refuse(format!("CSV error: {e}"));
|
||||
return;
|
||||
}
|
||||
}
|
||||
@@ -1000,22 +1012,21 @@ impl Effect for ImportJsonHeadless {
|
||||
let content = match app.storage.read(&self.path) {
|
||||
Ok(c) => c,
|
||||
Err(e) => {
|
||||
app.view_state.status_msg =
|
||||
format!("Cannot read '{}': {e}", self.path.display());
|
||||
app.refuse(format!("Cannot read '{}': {e}", self.path.display()));
|
||||
return;
|
||||
}
|
||||
};
|
||||
let value: serde_json::Value = match serde_json::from_slice(&content) {
|
||||
Ok(v) => v,
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("JSON parse error: {e}");
|
||||
app.refuse(format!("JSON parse error: {e}"));
|
||||
return;
|
||||
}
|
||||
};
|
||||
match json_import_records(&value, self.array_path.as_deref()) {
|
||||
Ok(recs) => (recs, value),
|
||||
Err(msg) => {
|
||||
app.view_state.status_msg = msg;
|
||||
app.refuse(msg);
|
||||
return;
|
||||
}
|
||||
}
|
||||
@@ -1054,7 +1065,7 @@ impl Effect for ImportJsonHeadless {
|
||||
};
|
||||
}
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("Import error: {e}");
|
||||
app.refuse(format!("Import error: {e}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1138,6 +1149,10 @@ pub fn set_status(msg: impl Into<String>) -> Box<dyn Effect> {
|
||||
Box::new(SetStatus(msg.into()))
|
||||
}
|
||||
|
||||
pub fn refuse(msg: impl Into<String>) -> Box<dyn Effect> {
|
||||
Box::new(Refuse(msg.into()))
|
||||
}
|
||||
|
||||
pub fn change_mode(mode: AppMode) -> Box<dyn Effect> {
|
||||
Box::new(ChangeMode(mode))
|
||||
}
|
||||
@@ -2053,6 +2068,41 @@ mod tests {
|
||||
assert_eq!(app.view_state.status_msg, "next-batch");
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/cli.md`.
|
||||
#[test]
|
||||
fn a_refusal_marks_only_its_own_batch() {
|
||||
let mut app = test_app();
|
||||
app.apply_effects(vec![Box::new(AddItem {
|
||||
category: "Nonexistent".into(),
|
||||
item: "Thing".into(),
|
||||
})]);
|
||||
assert!(app.last_batch_refused(), "{:?}", app.view_state.status_msg);
|
||||
app.apply_effects(vec![Box::new(SetStatus("a report".into()))]);
|
||||
assert!(
|
||||
!app.last_batch_refused(),
|
||||
"a report after a refusal inherited the refusal"
|
||||
);
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/cli.md`.
|
||||
#[test]
|
||||
fn a_chain_with_nowhere_to_go_aborts_without_refusing() {
|
||||
let mut app = test_app();
|
||||
app.apply_effects(vec![Box::new(AbortChain)]);
|
||||
assert!(app.last_batch_aborted(), "the fixture must abort");
|
||||
assert!(!app.last_batch_refused());
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/cli.md`.
|
||||
#[test]
|
||||
fn a_failed_save_is_a_refusal_as_well_as_an_abort() {
|
||||
let mut app = test_app();
|
||||
assert!(app.model_state.file_path.is_none());
|
||||
app.apply_effects(vec![Box::new(Save)]);
|
||||
assert!(app.last_batch_aborted());
|
||||
assert!(app.last_batch_refused());
|
||||
}
|
||||
|
||||
/// BUG (improvise-8tk): `Save` reported failure only to the status bar, so
|
||||
/// the chained `Quit` landed anyway and the session was discarded.
|
||||
#[test]
|
||||
|
||||
@@ -26,6 +26,8 @@ const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[
|
||||
("HelpPageNext", Verdict::Command("HelpPage")),
|
||||
("HelpPagePrev", Verdict::Command("HelpPage")),
|
||||
("HelpPageSet", Verdict::Command("HelpPage")),
|
||||
// Its text; the refusal flag is one host batch's outcome (context/code/cli.md §10).
|
||||
("Refuse", Verdict::Command("SetStatus")),
|
||||
("Save", Verdict::Frame("Save")),
|
||||
(
|
||||
"SaveAs",
|
||||
|
||||
@@ -1233,6 +1233,81 @@ fn page_selections(app: &App) -> Vec<(String, Option<String>)> {
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// improvise-6p8q: the TUI yanked and pasted through the synthetic records key.
|
||||
/// See `context/test-evidence/clients.md`.
|
||||
#[test]
|
||||
fn yy_then_p_in_records_mode_writes_the_record_the_host_writes() {
|
||||
let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE);
|
||||
let two_records = || {
|
||||
let mut app = furnished_host();
|
||||
let south = north_revenue().with("Region", "South");
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.set_cell(south, improvise_protocol::CellValue::Number(900.0));
|
||||
app.rebuild_layout();
|
||||
reduce_key(&mut app, r);
|
||||
app
|
||||
};
|
||||
let (mut app, mut twin) = (two_records(), two_records());
|
||||
let revenue = (0..app.layout.col_count())
|
||||
.find(|&c| app.layout.col_label(c) == "Revenue")
|
||||
.expect("a Revenue column");
|
||||
let south = 1;
|
||||
assert_eq!(
|
||||
app.layout.record_row(south).unwrap().coords.get("Region"),
|
||||
Some("South")
|
||||
);
|
||||
for host in [&mut app, &mut twin] {
|
||||
host.cursor_mut().selected = (0, revenue);
|
||||
}
|
||||
let mut client = records_client(&app);
|
||||
|
||||
both_ends(&mut app, &mut twin, &mut client, "y");
|
||||
both_ends(&mut app, &mut twin, &mut client, "y");
|
||||
assert_eq!(
|
||||
(client.view.yanked.clone(), app.view_state.yanked.clone()),
|
||||
(
|
||||
Some(improvise_protocol::CellValue::Number(1200.0)),
|
||||
Some(improvise_protocol::CellValue::Number(1200.0))
|
||||
),
|
||||
"both ends yank North's stored Revenue"
|
||||
);
|
||||
|
||||
for host in [&mut app, &mut twin] {
|
||||
host.cursor_mut().selected = (south, revenue);
|
||||
}
|
||||
client.view.cursor_mut().selected = (south, revenue);
|
||||
both_ends(&mut app, &mut twin, &mut client, "p");
|
||||
assert_eq!(
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.get_cell(&north_revenue().with("Region", "South")),
|
||||
Some(&improvise_protocol::CellValue::Number(1200.0)),
|
||||
"the paste must land on South's real record"
|
||||
);
|
||||
|
||||
// Off the measure columns the two ends still yank one value.
|
||||
for label in ["Region", "+"] {
|
||||
let col = (0..app.layout.col_count())
|
||||
.find(|&c| app.layout.col_label(c) == label)
|
||||
.unwrap();
|
||||
for host in [&mut app, &mut twin] {
|
||||
host.cursor_mut().selected = (south, col);
|
||||
}
|
||||
client.view.cursor_mut().selected = (south, col);
|
||||
both_ends(&mut app, &mut twin, &mut client, "y");
|
||||
both_ends(&mut app, &mut twin, &mut client, "y");
|
||||
assert_eq!(client.view.yanked, app.view_state.yanked, "{label}");
|
||||
}
|
||||
assert_eq!(
|
||||
app.view_state.yanked,
|
||||
Some(improvise_protocol::CellValue::Text(String::new())),
|
||||
"the last yank must have run, over the blank creating column"
|
||||
);
|
||||
}
|
||||
|
||||
/// Both hosts driven into records mode the way a user gets there, so the
|
||||
/// `_Records` view and its `_Index`/`_Dim` axes are the real ones.
|
||||
fn records_pair() -> (App, App) {
|
||||
|
||||
+9
-5
@@ -329,15 +329,17 @@ fn get_import_data(paths: &[PathBuf]) -> Option<Value> {
|
||||
|
||||
// ── Headless command execution ───────────────────────────────────────────────
|
||||
|
||||
/// The `(columns, rows)` every headless run behaves as if it had.
|
||||
const HEADLESS_EXTENT: (u16, u16) = (80, 24);
|
||||
|
||||
fn run_headless_commands(cmds: &[String], file: &Option<PathBuf>) -> Result<()> {
|
||||
use improvise::command::key::{KeyCode, KeyModifiers};
|
||||
|
||||
let workbook = get_initial_workbook(file)?;
|
||||
let mut app = ui::app::App::new(workbook, file.clone());
|
||||
if let Ok((w, h)) = crossterm::terminal::size() {
|
||||
app.term_width = w;
|
||||
app.term_height = h;
|
||||
}
|
||||
// Fixed, never read off the terminal: a page's size reaches the document,
|
||||
// so a script must not depend on the window it ran in (improvise-up8g).
|
||||
(app.term_width, app.term_height) = HEADLESS_EXTENT;
|
||||
let mut exit_code = 0;
|
||||
// `Save` clears `dirty`, so a run that edits and then `:w`s elsewhere would
|
||||
// otherwise look unchanged — `context/code/cli.md`.
|
||||
@@ -379,7 +381,9 @@ fn run_headless_commands(cmds: &[String], file: &Option<PathBuf>) -> Result<()>
|
||||
if !said.trim().is_empty() {
|
||||
eprintln!("{said}");
|
||||
}
|
||||
if app.last_batch_aborted() {
|
||||
// Refused, not aborted: an advance with nowhere to go aborts after
|
||||
// its write landed (improvise-c5ip) — `context/code/cli.md` §10.
|
||||
if app.last_batch_refused() {
|
||||
exit_code = 1;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -440,3 +440,111 @@ fn a_malformed_commit_line_is_refused_whole() {
|
||||
"the first half of a refused line landed: {text}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Runs `improvise cmd` with no controlling terminal and the given `LINES`,
|
||||
/// so the only size the process can discover is the one handed to it here.
|
||||
#[cfg(unix)]
|
||||
fn run_cmd_in_a_terminal_of(rows: u16, cols: u16, args: &[&str], file: &Path) -> Output {
|
||||
use std::os::unix::process::CommandExt;
|
||||
let mut cmd = Command::new(bin());
|
||||
cmd.arg("cmd")
|
||||
.args(args)
|
||||
.arg("-f")
|
||||
.arg(file)
|
||||
.env("TERM", "xterm")
|
||||
.env("LINES", rows.to_string())
|
||||
.env("COLUMNS", cols.to_string())
|
||||
.stdin(std::process::Stdio::null());
|
||||
// SAFETY: `setsid` is async-signal-safe, and leaving the runner's terminal
|
||||
// is what makes `LINES` the only size the child can find.
|
||||
unsafe {
|
||||
cmd.pre_exec(|| {
|
||||
libc::setsid();
|
||||
Ok(())
|
||||
});
|
||||
}
|
||||
cmd.output().expect("the improvise binary runs")
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/cli.md`.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn a_script_writes_the_same_record_whatever_terminal_ran_it() {
|
||||
let scratch = Scratch::new("extent");
|
||||
let items: Vec<String> = (1..=60).map(|i| format!("R{i:02}")).collect();
|
||||
let seed = scratch.path("seed.improv");
|
||||
run_cmd(&["add-category Region"], &seed);
|
||||
run_cmd(&[&format!("add-items Region {}", items.join(" "))], &seed);
|
||||
let script = ["page-scroll 1 . hide-selected-row-item"];
|
||||
|
||||
let mut hidden = Vec::new();
|
||||
for (rows, cols) in [(24, 80), (60, 200)] {
|
||||
let doc = scratch.path(&format!("{rows}x{cols}.improv"));
|
||||
std::fs::copy(&seed, &doc).expect("a copy of the seed");
|
||||
let out = run_cmd_in_a_terminal_of(rows, cols, &script, &doc);
|
||||
assert!(out.status.success(), "{}", stderr_of(&out));
|
||||
hidden.push(hidden_line(&doc));
|
||||
}
|
||||
assert_ne!(
|
||||
hidden[0], "hidden: Region/R01",
|
||||
"the cursor never moved, so the page size was never read"
|
||||
);
|
||||
assert!(
|
||||
hidden[0].starts_with("hidden: Region/R"),
|
||||
"the script hid nothing, so agreement proves nothing: {hidden:?}"
|
||||
);
|
||||
assert_eq!(
|
||||
hidden[0], hidden[1],
|
||||
"the same script hid a different record at a different terminal size"
|
||||
);
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/cli.md`.
|
||||
#[test]
|
||||
fn a_commit_whose_advance_has_nowhere_to_go_still_exits_zero() {
|
||||
let scratch = Scratch::new("commit-tail");
|
||||
let doc = three_cells(&scratch, "tail.improv");
|
||||
let out = run_cmd(
|
||||
&["move-selection 2 0 . commit-cell-edit editing 9 Region/South"],
|
||||
&doc,
|
||||
);
|
||||
assert_eq!(
|
||||
value_of(&doc, "South").as_deref(),
|
||||
Some("9"),
|
||||
"the write did not land"
|
||||
);
|
||||
assert!(
|
||||
out.status.success(),
|
||||
"the write landed and was saved, yet the run exited non-zero: {:?}",
|
||||
stderr_of(&out)
|
||||
);
|
||||
|
||||
let empty = scratch.path("empty.improv");
|
||||
let out = run_cmd(
|
||||
&["add-category Foo . commit-cell-edit editing 5 Foo/x"],
|
||||
&empty,
|
||||
);
|
||||
assert!(
|
||||
out.status.success(),
|
||||
"a commit on an empty grid landed and exited non-zero: {:?}",
|
||||
stderr_of(&out)
|
||||
);
|
||||
}
|
||||
|
||||
/// Evidence: `context/test-evidence/cli.md`.
|
||||
#[test]
|
||||
fn a_refusal_that_only_writes_a_status_exits_non_zero() {
|
||||
let scratch = Scratch::new("refusal-exit");
|
||||
let doc = seeded(&scratch, "m.improv");
|
||||
let out = run_cmd(&["add-item Nonexistent Thing"], &doc);
|
||||
assert!(
|
||||
stderr_of(&out).contains("Nonexistent"),
|
||||
"the refusal was not reported, so its exit code proves nothing: {:?}",
|
||||
stderr_of(&out)
|
||||
);
|
||||
assert!(
|
||||
!out.status.success(),
|
||||
"a refused command exited 0: {:?}",
|
||||
stderr_of(&out)
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user