Compare commits
5 Commits
1c184a5cc2
...
800348ad11
| Author | SHA1 | Date | |
|---|---|---|---|
| 800348ad11 | |||
| 8d51ad70f2 | |||
| 0d8e6a908b | |||
| ffe8aed6b4 | |||
| 487f0b6fef |
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.
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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`.
|
||||
|
||||
@@ -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",
|
||||
|
||||
+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