diff --git a/context/code/cli.md b/context/code/cli.md index 5add820a..39a32083 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -425,3 +425,116 @@ check. The rest come from `build_model_with_warnings` as an `Err`. All of them reach `main` as exit 1 before `persistence::save`. So "writes nothing" holds by statement order, not by where the refusal is raised. The headless tests check it by asserting that the output path does not exist. + +## 14 — the command language reads `|pipe quoting|`, and a surplus argument is refused (improvise-31oe.3) + +`create-view |By area|` used to make a view named `|By` and drop `area|` without +a word. Two separate defects, fixed separately. + +**The quoting.** `parse.rs` now has one lexer, `lex`, and everything else is +built on it: `parse_line_with`, `segments` and `parse_segment_interactive`. The +rules are: + +- A `|…|` piece is read by `improvise_formula::quote::unescape_pipe_body`, the + one home of pipe quoting (gotcha 34). So `\|`, `\\` and `\n` mean exactly what + they mean in `.improv` and in formulas. The lexer only finds the closing pipe: + a backslash takes the next character with it, so `\|` does not close. +- A piece may sit **anywhere in a word**, and the pieces and bare text are + joined. So a coordinate quotes each half, `|Sales Region|/|East Coast|`, the + way a `.improv` data line quotes each name. The alternative, a pipe only at + the start of a word, would have left no way to spell a coordinate with a + space in it. +- `"…"` is **kept**, verbatim, with no escapes, and a `|` inside it stays a + `|`. It is not a workaround to retire. Formula text carries its own pipes: + `add-formula "Net = |Gross Rev| - Cost"` must reach `parse_formula` with the + pipes on. If the line had pipes only, every pipe in a formula would need + escaping. Its old leniency is kept too: a `"` that is never closed runs to + the end of the line. A `|` that is never closed is refused, naming the text + from the open pipe on, because a name is never meant to run to the end. +- The separator is a **bare** `.` word. A quoted `|.|` or `"."` is an argument, + and a `.` inside `|a . b|` does not split the line. The old `split_on_dot` + was a second scanner that knew double quotes and not pipes. With one lexer + there is no second scanner to disagree. +- A comment line (`#`, `//`) is not lexed, so an unmatched pipe in a comment is + not an error. + +`segments` now returns `Result`, because an unterminated pipe is a fact about +the whole line. `main.rs` refuses the line before running any of it, as §7 does +for a parse error, and `ExecuteCommand` refuses it on the status line. + +What is still not spellable: an item whose name contains `/`. The coordinate is +split on the first `/` **after** unquoting, so `|a/b|/x` is still read as +category `a` and item `b/x`. Closing that needs the lexer to hand structured +coordinates to the commands, which is a larger change than this ticket. + +**The arity.** Every registration now declares `MaxArgs::AtMost(n)` or +`MaxArgs::Any` beside its prototype. `CmdRegistry::parse` and +`CmdRegistry::interactive` apply it after construction succeeds. So the rule is +written once and every command gets it, and a new registration cannot compile +without a declaration. The refusal names the command, the limit and every +surplus argument (`create-view: takes at most 1 argument(s), got 2; unexpected +"area"`). A headless run exits non-zero through the §7 parse gate, and the `:` +line refuses on the status line. + +It runs **after** the command's own parse, deliberately. A command whose own +refusal states its shape keeps it: `add-formula`'s "quote the formula" +(improvise-z05h) is more use than an arity, and it is what `formula Product +Total = Widget + Gadget` still says. + +`MaxArgs::Any` is a claim, and the list of commands making it is short: +`add-items`, `set-cell`, `set-format` (it joins its words), `clear-cell`, +`yank`, `paste`, `drill-into-cell` and `commit-cell-edit`. Each takes a list of +items or coordinates. `no_command_silently_drops_an_argument_it_was_given` names +that list, so widening another command to `Any` has to add it there. + +## 15 — a command that names something the document lacks is refused (improvise-31oe.2) + +`set-axis Regoin row`, `set-page Region Nowhere` and `set-axis Region sideways` +all exited 0 and said nothing. §10's rule applies: nothing changed, so the +request was not met. + +- **An unreadable axis is an argument error.** `set-axis` now checks its axis + with `parse_axis` when it is parsed, so a script is refused before any of it + runs. `execute` still refuses an axis it cannot read rather than returning + nothing. That arm cannot be reached through the registry. +- **An unknown category, item, group or view is a refusal**, from `RequireKnown` + (below), because only the document knows what it holds. `effect::unknown_name` + asks the active view (`try_axis_of`) for a category. That is exactly the + condition under which `View::set_axis` was a silent no-op. It asks + `effective_item_names` for an item, so a formula target is a known `_Measure` + item. It asks both an item's own group and the category's `groups` for a + group. `add_item_in_group` registers no `Group`, and a parent group has no + items, so either one alone misses a real group. A view is asked of + `workbook.views`. + +The siblings fixed with it: `hide-item` and `show-item` (hiding an unknown item +wrote it into `hidden_items`), `toggle-group`, and `switch-view` (the effect +ignores a view it lacks, and says nothing). `set-page` with an unknown item was +worse than a no-op: it wrote the name into `page_selections`. + +The check runs at **apply** time, as an effect of its own. `execute` runs +against one context built before the line ran, and a `:` line builds every +segment up front, so a check in `execute` refused +`create-view Second . switch-view Second` and +`add-item Type Tools . set-page Type Tools` after their first halves had landed, +while `improvise cmd` (which rebuilds the context per segment) accepted them. +Each of these commands therefore returns `RequireKnown(needs)` followed by the +real effect. `RequireKnown` refuses and aborts the batch (gotcha 21), so the +real effect never applies. It is a `ViewOnly` effect that writes nothing and is +off the wire on purpose: the wire's own effects (`model_effects`, gotcha 38) +stay non-refusing, since a refusing effect there becomes a frame rejection +(`MutationRefused`). That is arguably right, but it changes the protocol, and +a browser composes these only from names it was sent. It is left for a ticket +of its own. + +A refused name stops the rest of a `:` line, because the abort ends the one batch +the whole line runs as. Headless runs each segment as its own batch, so an abort +never crosses a `.` there: `:set-page Type Nowhere . add-item Type X` adds +nothing, while `improvise cmd` on the same line exits 1 and still adds `X`. +`load`/`Save` already behaved this way. The two front ends' chain semantics are +left for a ticket of their own. + +The interactive cursor fallbacks of `clear-cell`, `yank`, `paste` and +`drill-into-cell` answer only for coordinates that were OMITTED +(`only_when_omitted`). Coordinates that were given and are malformed are +refused, never replaced by the cursor's cell. diff --git a/context/code/command.md b/context/code/command.md index a89a6621..843d0f0d 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2996,8 +2996,10 @@ ride in `ViewState::transient_prefix`, so they must live where the wire types do and the re-export keeps `crate::command::key::…` paths working. `parse.rs` is the quasi-lisp prefix parser: `word arg1 arg2 …`, several commands per -line separated by `.`, coordinate pairs written `Category/Item`, and quoted strings -(`"Profit = Revenue - Cost"`). +line separated by `.`, coordinate pairs written `Category/Item`, verbatim quoted +strings (`"Profit = Revenue - Cost"`) and `|pipe-quoted names|` read by +`improvise_formula::quote`. One lexer serves every entry point, so the separator +and the quoting cannot disagree (improvise-31oe.3, `context/code/cli.md` §14). ### `cmd/core.rs` @@ -3014,6 +3016,12 @@ writes any other field, so an accepted argument list is never overridden by context (improvise-2bac, improvise-yf7p; the argument is `context/code/cli.md` §8). `register_pure` is the case with no fallback: both arms are `parse`. +Every registration also declares its `MaxArgs`, and the registry refuses surplus +arguments on both arms after the command is built. `register_nullary` declares +`AtMost(0)` for itself, and an `effect_cmd!` declares its limit in the macro +call as `MAX_ARGS`. So the arity rule is written once and is total over the +registry (improvise-31oe.3, `context/code/cli.md` §14). + `write_buffer` is the effect that writes a named buffer — the mirror of `read_buffer`, and the only place that knows "search" is not an entry in the buffers map but its own `ViewState` field (improvise-gdl, gotcha 11). That knowledge used to @@ -3372,6 +3380,14 @@ The parseable model-mutation commands are thin `Cmd` wrappers around effects, constructible from string args, sharing the same execution path as keymap-dispatched commands (gotcha 16). +A wrapper that names a category, item, group or view returns +`effect::RequireKnown(Needs)` followed by its real effect (improvise-31oe.2). +`RequireKnown` asks `unknown_name` against the live app when the batch applies +and refuses and aborts if the document lacks the name, so a `:` line sees what +its earlier segments made. The effects themselves stay silent about unknown +names, because they are also the wire's mutation path. Why the check is a +separate effect: `context/code/cli.md` §15. + ### `cmd/wizard.rs` (improvise-2wm, improvise-907) Import-wizard key handling. The wizard is a small step machine, and which key means diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 7f9b2233..ef103f13 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -510,6 +510,116 @@ directly imports without any `File`. The rival that merges every all-CSV import, lone files included, fails that half ("a lone file keeps today's shape"). So the test separates "a folder merges" from "everything merges". +### a_pipe_quoted_argument_is_one_argument_without_its_pipes + +improvise-31oe.3. Red before the fix: the old tokenizer split on whitespace +only, so `create-view |By area|` built `CreateViewCmd(["|By", "area|"])`. It +asserts the whole built command, so a lexer that joined the words but kept the +pipes fails as well as one that split them. The second case has two quoted +arguments side by side, so a lexer that reads one pipe piece per line fails. +Rival: `bare_word` pushing `|` as a plain character. Fails here and in +`a_coordinate_may_pipe_quote_either_half`. + +### a_pipe_quoted_argument_reads_the_escapes_improv_does + +The three escapes `pipe_quote` writes, `\|`, `\\` and `\n`, each in its own +case. Red before the fix. Rival: `pipe_body` returning the raw body without +`unescape_pipe_body`. Fails here, at the first case, and nowhere else in the +crate, which is why the case exists. A lexer that treated `\|` as a closing +pipe would also fail the first case, because the pipe after `b` then opens a +piece that never closes. + +### a_command_given_more_arguments_than_it_takes_is_refused + +improvise-31oe.3's second half. Red before the fix: `create-view By area` +parsed and dropped `area`. It asserts the refusal opens with the command's name +and quotes the surplus word, and that the pipe-quoted spelling of the same line +is the control that parses. Rival: `MaxArgs::admit` answering `Ok` always. +Fails here, in `no_command_silently_drops_an_argument_it_was_given`, and in +`a_colon_line_naming_something_missing_refuses_on_the_status_line`. + +### no_command_silently_drops_an_argument_it_was_given + +The rule is one rule only if every registration obeys it, so this enumerates +every registered name against every candidate argument list, on both arms. For +each list a command accepts, it appends **four** surplus words. One would be +absorbed by an optional argument (`add-formula ["1"]` plus one more is a valid +two-argument call), and four is more than any optional span in the registry. +The last word must be named in the refusal. The exceptions are stated, not +inferred: the `Any` list, and three commands whose own shape refusal runs +first. Rivals, each run: declaring `add-item-in-group` as `Any` fails here, +naming the dropped list. Removing the check from the interactive arm alone +fails here and in the `:` line test. A test of the pure arm alone would have +missed that. + +### a_command_naming_something_the_document_lacks_is_a_refusal + +improvise-31oe.2 and its siblings: `set-axis`, `set-page`, `hide-item`, +`show-item`, `toggle-group` and `switch-view`, each naming a missing category, +item, group or view. Red before the fix at the first row, `set-axis Regoin +column`, with an empty status. Each row asserts the refusal flag, the missing +name in the message, and that the document's `.improv` text and the active view +are unchanged. The last two catch a refusal written after the write. The +control is `a_command_naming_what_the_document_has_is_not_refused`, whose group +row is the one that caught a group check reading only `Category::groups`: +`add_item_in_group` registers no `Group`. The checks are `effect::unknown_name`, +run by `RequireKnown` when the batch applies. Rivals, each run: the +`Needs::Item` arm answering `None` fails at `set-page ["Type", "Nowhere"]`. The +category test (`try_axis_of(..).is_none()`) never firing fails at the first +row. The `Needs::View` arm answering `None` fails at the `switch-view` row. +`RequireKnown` placed after the write, or refusing without `refuse_and_stop`'s +abort, fails the document-unchanged assertion. + +### a_colon_line_naming_something_missing_refuses_on_the_status_line + +The TUI path for both tickets, through `ExecuteCommand` and `App::apply_effects`, +not through the command alone. It asserts the refusal flag, the name on the +status line, and the mode back at Normal. Rivals: an unknown category accepted +fails at `:set-axis Regoin row`, and the arity check removed from the +interactive arm fails at `:create-view By area`. + +### an_unknown_category_axis_or_item_exits_non_zero_and_names_it + +The three repros from improvise-31oe.2, at the process boundary. Red against +the `gh/main` source with this test file: `set-axis Regoin row` exited 0 with an +empty stderr. The control, `set-page Region East`, must exit 0. Without it, a +fix that refused every `set-page` would pass. + +### a_pipe_quoted_name_reaches_the_document_whole + +improvise-31oe.3's repro at the process boundary: the saved document must carry +`## View: By area` and no `|By`. The second half, `create-view By area`, must +exit non-zero naming `"area"`. Red against the `gh/main` source: the document +carried the view `|By`. + +### command::cmd::registry::tests::a_malformed_coordinate_is_refused_not_dropped + +Codex's P2 on PR #316: `set-cell` and `parse_cell_key_from_args` (`clear-cell`, +`yank`, `paste`, `drill-into-cell`, `commit-cell-edit`) built coordinates with +`filter_map(split_once('/'))`, so `set-cell 1 Type/Food unexpected` dropped +`unexpected` without a word. The test names every caller, a half-empty +coordinate, and a control line that must still parse. Rival: the `filter_map` +restored in `parse_cell_key_from_args` fails at the first line, +`set-cell 1 Type/Food unexpected dropped "unexpected" without a word`. + +### command::cmd::text_buffer::tests::colon_refuses_a_malformed_coordinate_and_leaves_the_cursor_cell_alone + +The PR #316 review's blocker: on `:`, `parse_cell_key_from_args` failing sent +`clear-cell`, `yank`, `paste` and `drill-into-cell` into the cursor fallback, +which ignores its arguments. `:clear-cell Type/Clothing Month/Feb junk` cleared +the cursor's Food/Jan without a word. The test runs each line through the full +`:` path and asserts a refusal naming `"junk"`, both cells intact, nothing +yanked and no drill opened. Rival: `only_when_omitted` made unconditionally +`Ok` fails it at the first line. + +### command::cmd::text_buffer::tests::colon_lets_a_later_segment_use_what_an_earlier_one_created + +The review's second blocker: a `:` line is built before any segment runs, so the +name checks in `execute` refused `create-view Second . switch-view Second` and +`add-item Type Tools . set-page Type Tools` after the first half landed. The +checks now run when the effect applies. Rival: the checks back in `execute` +fails both lines with `No view named "Second"` and `Unknown item 'Tools'`. + ### a_strict_import_that_skips_a_record_refuses_and_writes_nothing improvise-8jk6.6, the user's ruling on PR #321. Written before `--strict` diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 7b9808e9..876e6b59 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -250,12 +250,37 @@ pub trait AtCursor: Cmd { fn cursor_mut(&mut self) -> &mut CursorState; } +/// How many arguments a command takes. The registry refuses any beyond it, for +/// every command in one place — `context/code/cli.md` §14. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum MaxArgs { + AtMost(usize), + Any, +} + +impl MaxArgs { + fn admit(self, args: &[String]) -> Result<(), String> { + match self { + MaxArgs::AtMost(n) if args.len() > n => { + let extra: Vec = args[n..].iter().map(|a| format!("{a:?}")).collect(); + Err(format!( + "takes at most {n} argument(s), got {}; unexpected {}", + args.len(), + extra.join(" ") + )) + } + MaxArgs::AtMost(_) | MaxArgs::Any => Ok(()), + } + } +} + type BoxParseFn = Box Result, String>>; type BoxInteractiveFn = Box Result, String>>; /// A registered command entry with both text and interactive constructors. struct CmdEntry { name: &'static str, + max: MaxArgs, parse: BoxParseFn, interactive: BoxInteractiveFn, } @@ -295,10 +320,11 @@ impl CmdRegistry { pub fn register( &mut self, prototype: &C, + max: MaxArgs, parse: fn(&[String]) -> Result, fallback: Option>, ) { - self.push(prototype.name(), parse, fallback, |_, _| {}); + self.push(prototype.name(), max, parse, fallback, |_, _| {}); } /// As [`register`](Self::register), then the interactive arm moves the @@ -306,10 +332,11 @@ impl CmdRegistry { pub fn register_at_cursor( &mut self, prototype: &C, + max: MaxArgs, parse: fn(&[String]) -> Result, fallback: Option>, ) { - self.push(prototype.name(), parse, fallback, |cmd, ctx| { + self.push(prototype.name(), max, parse, fallback, |cmd, ctx| { *cmd.cursor_mut() = CursorState::from_ctx(ctx); }); } @@ -317,12 +344,14 @@ impl CmdRegistry { fn push( &mut self, name: &'static str, + max: MaxArgs, parse: fn(&[String]) -> Result, fallback: Option>, at_cursor: fn(&mut C, &CmdContext), ) { self.entries.push(CmdEntry { name, + max, parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), interactive: Box::new(move |args, ctx| { let mut built = match (parse(args), fallback) { @@ -338,9 +367,10 @@ impl CmdRegistry { /// Register a command that needs no interactive context: BOTH arms are /// `parse`, so neither can demote the other — `context/code/command.md`. - pub fn register_pure(&mut self, prototype: &dyn Cmd, parse: ParseFn) { + pub fn register_pure(&mut self, prototype: &dyn Cmd, max: MaxArgs, parse: ParseFn) { self.entries.push(CmdEntry { name: prototype.name(), + max, parse: Box::new(parse), interactive: Box::new(move |args, _ctx| parse(args)), }); @@ -352,6 +382,7 @@ impl CmdRegistry { let name = f().name(); self.entries.push(CmdEntry { name, + max: MaxArgs::AtMost(0), parse: Box::new(move |_| Ok(f())), interactive: Box::new(move |_, _| Ok(f())), }); @@ -364,7 +395,9 @@ impl CmdRegistry { let canonical = self.resolve(name); for e in &self.entries { if e.name == canonical { - return (e.parse)(args).map_err(|err| format!("{name}: {err}")); + return (e.parse)(args) + .and_then(|cmd| e.max.admit(args).map(|()| cmd)) + .map_err(|err| format!("{name}: {err}")); } } Err(format!("Unknown command: {name}")) @@ -381,7 +414,7 @@ impl CmdRegistry { let name = self.resolve(name); for e in &self.entries { if e.name == name { - return (e.interactive)(args, ctx); + return (e.interactive)(args, ctx).and_then(|cmd| e.max.admit(args).map(|()| cmd)); } } Err(format!("Unknown command: {name}")) @@ -401,16 +434,31 @@ pub(super) fn require_args(args: &[String], n: usize) -> Result<(), String> { } } -/// Parse Cat/Item coordinate args into a CellKey. -pub(super) fn parse_cell_key_from_args(args: &[String]) -> crate::model::cell::CellKey { - let coords: Vec<(String, String)> = args +/// Parse Cat/Item coordinate args into a CellKey, refusing any token that is not +/// one rather than dropping it — `context/code/cli.md` §14. +pub(super) fn parse_cell_key_from_args( + args: &[String], +) -> Result { + let coords = args .iter() - .filter_map(|s| { - let (cat, item) = s.split_once('/')?; - Some((cat.to_string(), item.to_string())) + .map(|s| match s.split_once('/') { + Some((cat, item)) if !cat.is_empty() && !item.is_empty() => { + Ok((cat.to_string(), item.to_string())) + } + _ => Err(format!("{s:?} is not a Category/Item coordinate")), }) - .collect(); - crate::model::cell::CellKey::new(coords) + .collect::, _>>()?; + Ok(crate::model::cell::CellKey::new(coords)) +} + +/// A cursor fallback answers only for coordinates that were OMITTED; given and +/// wrong, they are refused, never the cursor's cell — `context/code/cli.md` §14. +pub(super) fn only_when_omitted(args: &[String]) -> Result<(), String> { + if args.is_empty() { + return Ok(()); + } + parse_cell_key_from_args(args)?; + Err("takes Category/Item coordinates, or none to use the cursor".to_string()) } /// Read the current value of a named buffer from context. diff --git a/crates/improvise-command/src/command/cmd/effect_cmds.rs b/crates/improvise-command/src/command/cmd/effect_cmds.rs index 44d93e32..4ed3ffa4 100644 --- a/crates/improvise-command/src/command/cmd/effect_cmds.rs +++ b/crates/improvise-command/src/command/cmd/effect_cmds.rs @@ -1,13 +1,13 @@ use crate::model::cell::CellValue; use crate::ui::effect::{self, Effect, ViewTarget}; -use crate::view::Axis; -use super::core::{Cmd, CmdContext, require_args}; +use super::core::{Cmd, CmdContext, MaxArgs, parse_axis, parse_cell_key_from_args, require_args}; #[cfg(test)] mod tests { use super::*; use crate::command::cmd::test_helpers::*; + use crate::view::Axis; #[test] fn add_category_cmd_produces_add_category_effect() { @@ -59,7 +59,7 @@ mod tests { let ctx = make_ctx(&m, &layout, ®); let cmd = SetAxisCmd(vec!["Type".to_string(), "col".to_string()]); let effects = cmd.execute(&ctx); - assert_eq!(effects.len(), 1); + assert_eq!(effects.len(), 2, "the name check, then the write"); let mut app = crate::ui::app::App::new(two_cat_model(), None); app.apply_effects(effects); let view = app.model_state.workbook.active_view(); @@ -95,6 +95,104 @@ mod tests { assert!(present.model_state.workbook.model.formulas().is_empty()); } + /// Run `name args` built by the pure arm against a fresh `App` on `wb`. + fn run_on(wb: &crate::workbook::Workbook, name: &str, args: &[&str]) -> crate::ui::app::App { + let args: Vec = args.iter().map(|s| s.to_string()).collect(); + let mut app = crate::ui::app::App::new(wb.clone(), None); + let cmd = crate::command::cmd::registry::default_registry() + .parse(name, &args) + .unwrap_or_else(|e| panic!("`{name} {args:?}` must build: {e}")); + let effects = { + let ctx = app.cmd_context( + crate::command::key::KeyCode::Null, + crate::command::key::KeyModifiers::NONE, + ); + cmd.execute(&ctx) + }; + app.apply_effects(effects); + app + } + + /// A document where every sibling below has something real to name. + fn named_things() -> crate::workbook::Workbook { + let mut wb = two_cat_model(); + wb.add_item_in_group("Type", "Tools", "Hardware").unwrap(); + wb.create_view("Second"); + wb + } + + /// improvise-31oe.2 and its siblings: each names something missing, is + /// refused naming it, and changes nothing. Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_command_naming_something_the_document_lacks_is_a_refusal() { + let wb = named_things(); + let cases: [(&str, &[&str], &str); 10] = [ + ("set-axis", &["Regoin", "column"], "Regoin"), + ("set-page", &["Regoin", "Food"], "Regoin"), + ("set-page", &["Type", "Nowhere"], "Nowhere"), + ("hide-item", &["Regoin", "Food"], "Regoin"), + ("hide-item", &["Type", "Nowhere"], "Nowhere"), + ("show-item", &["Regoin", "Food"], "Regoin"), + ("show-item", &["Type", "Nowhere"], "Nowhere"), + ("toggle-group", &["Regoin", "Hardware"], "Regoin"), + ("toggle-group", &["Type", "Nowhere"], "Nowhere"), + ("switch-view", &["Nowhere"], "Nowhere"), + ]; + let untouched = crate::persistence::format_md(&wb); + for (name, args, missing) in cases { + let app = run_on(&wb, name, args); + let said = &app.view_state.status_msg; + assert!( + app.last_batch_refused(), + "`{name} {args:?}` was not refused: {said:?}" + ); + assert!( + said.contains(missing), + "`{name} {args:?}` did not name {missing}: {said:?}" + ); + assert_eq!( + crate::persistence::format_md(&app.model_state.workbook), + untouched, + "`{name} {args:?}` changed the document" + ); + assert_eq!( + app.view_state.active_view, wb.active_view, + "`{name}` moved the view" + ); + } + } + + /// The controls: the same commands naming what IS there are not refused. + #[test] + fn a_command_naming_what_the_document_has_is_not_refused() { + let wb = named_things(); + let cases: [(&str, &[&str]); 6] = [ + ("set-axis", &["Type", "column"]), + ("set-page", &["Type", "Clothing"]), + ("hide-item", &["Type", "Clothing"]), + ("show-item", &["Type", "Clothing"]), + ("toggle-group", &["Type", "Hardware"]), + ("switch-view", &["Second"]), + ]; + for (name, args) in cases { + let app = run_on(&wb, name, args); + assert!( + !app.last_batch_refused(), + "`{name} {args:?}` was refused: {:?}", + app.view_state.status_msg + ); + } + } + + /// An axis keyword is an argument error, caught before anything runs. + #[test] + fn set_axis_with_an_unknown_axis_is_an_argument_error() { + let err = crate::command::parse_line("set-axis Type sideways") + .expect_err("sideways is not an axis"); + assert!(err.starts_with("set-axis:"), "names the command: {err}"); + assert!(err.contains("sideways"), "names the axis: {err}"); + } + #[test] fn write_cmd_without_args_saves() { let m = two_cat_model(); @@ -129,7 +227,7 @@ mod tests { // Thin Cmd wrappers around effects, on the same path as keymap dispatch. macro_rules! effect_cmd { - ($name:ident, $cmd_name:expr, $parse:expr, $exec:expr) => { + ($name:ident, $cmd_name:expr, $max:expr, $parse:expr, $exec:expr) => { #[derive(Debug)] pub struct $name(pub Vec); impl Cmd for $name { @@ -143,6 +241,7 @@ macro_rules! effect_cmd { } } impl $name { + pub const MAX_ARGS: MaxArgs = $max; pub fn parse(args: &[String]) -> Result, String> { #[allow(clippy::redundant_closure_call)] ($parse)(args)?; @@ -152,9 +251,16 @@ macro_rules! effect_cmd { }; } +/// `effect`, preceded by the check that what it names exists, run at APPLY time: +/// a `:` line is built before any of it runs — `context/code/cli.md` §15. +fn unless_unknown(needs: effect::Needs, effect: impl Effect + 'static) -> Vec> { + vec![Box::new(effect::RequireKnown(needs)), Box::new(effect)] +} + effect_cmd!( AddCategoryCmd, "add-category", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::AddCategory(args[0].clone()))] @@ -164,6 +270,7 @@ effect_cmd!( effect_cmd!( AddItemCmd, "add-item", + MaxArgs::AtMost(2), |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::AddItem { @@ -176,6 +283,7 @@ effect_cmd!( effect_cmd!( AddItemsCmd, "add-items", + MaxArgs::Any, |args: &[String]| { if args.len() < 2 { Err("add-items requires a category and at least one item".to_string()) @@ -200,6 +308,7 @@ effect_cmd!( effect_cmd!( AddItemInGroupCmd, "add-item-in-group", + MaxArgs::AtMost(3), |args: &[String]| require_args(args, 3), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::AddItemInGroup { @@ -213,11 +322,12 @@ effect_cmd!( effect_cmd!( SetCellCmd, "set-cell", + MaxArgs::Any, |args: &[String]| { if args.len() < 2 { Err("set-cell requires a value and at least one Cat/Item coordinate".to_string()) } else { - Ok(()) + parse_cell_key_from_args(&args[1..]).map(|_| ()) } }, |args: &Vec, _ctx: &CmdContext| -> Vec> { @@ -226,21 +336,17 @@ effect_cmd!( } else { CellValue::Text(args[0].clone()) }; - let coords: Vec<(String, String)> = args[1..] - .iter() - .filter_map(|s| { - let (cat, item) = s.split_once('/')?; - Some((cat.to_string(), item.to_string())) - }) - .collect(); - let key = crate::model::cell::CellKey::new(coords); - vec![Box::new(effect::SetCell(key, value))] + match parse_cell_key_from_args(&args[1..]) { + Ok(key) => vec![Box::new(effect::SetCell(key, value))], + Err(why) => vec![effect::refuse(why)], + } } ); effect_cmd!( AddFormulaCmd, "add-formula", + MaxArgs::AtMost(2), |args: &[String]| { if args.is_empty() || args.len() > 2 { // The arity alone does not tell anyone to quote — improvise-z05h. @@ -269,6 +375,7 @@ effect_cmd!( effect_cmd!( ClearBufferCmd, "clear-buffer", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![super::core::write_buffer(&args[0], String::new())] @@ -278,6 +385,7 @@ effect_cmd!( effect_cmd!( RemoveFormulaCmd, "remove-formula", + MaxArgs::AtMost(2), |args: &[String]| require_args(args, 2), |args: &Vec, ctx: &CmdContext| -> Vec> { let (category, target) = (&args[0], &args[1]); @@ -298,6 +406,7 @@ effect_cmd!( effect_cmd!( CreateViewCmd, "create-view", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::CreateView(args[0].clone()))] @@ -307,6 +416,7 @@ effect_cmd!( effect_cmd!( DeleteViewCmd, "delete-view", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::DeleteView(args[0].clone()))] @@ -316,87 +426,111 @@ effect_cmd!( effect_cmd!( SwitchViewCmd, "switch-view", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { - vec![Box::new(effect::SwitchView(args[0].clone()))] + unless_unknown( + effect::Needs::View(args[0].clone()), + effect::SwitchView(args[0].clone()), + ) } ); effect_cmd!( SetAxisCmd, "set-axis", - |args: &[String]| require_args(args, 2), + MaxArgs::AtMost(2), + |args: &[String]| { + require_args(args, 2)?; + parse_axis(&args[1]).map(|_| ()) + }, |args: &Vec, _ctx: &CmdContext| -> Vec> { - let axis = match args[1].to_lowercase().as_str() { - "row" => Axis::Row, - "column" | "col" => Axis::Column, - "page" => Axis::Page, - "none" => Axis::None, - _ => return vec![], // parse step already validated - }; - vec![Box::new(effect::SetAxis { - view: ViewTarget::Active, - category: args[0].clone(), - axis, - })] + match parse_axis(&args[1]) { + Ok(axis) => unless_unknown( + effect::Needs::Category(args[0].clone()), + effect::SetAxis { + view: ViewTarget::Active, + category: args[0].clone(), + axis, + }, + ), + Err(why) => vec![effect::refuse(why)], + } } ); effect_cmd!( SetPageCmd, "set-page", + MaxArgs::AtMost(2), |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { - vec![Box::new(effect::SetPageSelection { - view: ViewTarget::Active, - category: args[0].clone(), - item: args[1].clone(), - })] + unless_unknown( + effect::Needs::Item(args[0].clone(), args[1].clone()), + effect::SetPageSelection { + view: ViewTarget::Active, + category: args[0].clone(), + item: args[1].clone(), + }, + ) } ); effect_cmd!( ToggleGroupCmd, "toggle-group", + MaxArgs::AtMost(2), |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { - vec![Box::new(effect::ToggleGroup { - view: ViewTarget::Active, - category: args[0].clone(), - group: args[1].clone(), - })] + unless_unknown( + effect::Needs::Group(args[0].clone(), args[1].clone()), + effect::ToggleGroup { + view: ViewTarget::Active, + category: args[0].clone(), + group: args[1].clone(), + }, + ) } ); effect_cmd!( HideItemCmd, "hide-item", + MaxArgs::AtMost(2), |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { - vec![Box::new(effect::HideItem { - view: ViewTarget::Active, - category: args[0].clone(), - item: args[1].clone(), - })] + unless_unknown( + effect::Needs::Item(args[0].clone(), args[1].clone()), + effect::HideItem { + view: ViewTarget::Active, + category: args[0].clone(), + item: args[1].clone(), + }, + ) } ); effect_cmd!( ShowItemCmd, "show-item", + MaxArgs::AtMost(2), |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { - vec![Box::new(effect::ShowItem { - view: ViewTarget::Active, - category: args[0].clone(), - item: args[1].clone(), - })] + unless_unknown( + effect::Needs::Item(args[0].clone(), args[1].clone()), + effect::ShowItem { + view: ViewTarget::Active, + category: args[0].clone(), + item: args[1].clone(), + }, + ) } ); effect_cmd!( SaveAsCmd, "save-as", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::SaveAs(std::path::PathBuf::from(&args[0])))] @@ -406,6 +540,7 @@ effect_cmd!( effect_cmd!( SetFormatCmd, "set-format", + MaxArgs::Any, |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::SetNumberFormat( @@ -418,6 +553,7 @@ effect_cmd!( effect_cmd!( ImportCmd, "import", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::StartImportWizard(args[0].clone()))] @@ -427,6 +563,7 @@ effect_cmd!( effect_cmd!( ExportCmd, "export", + MaxArgs::AtMost(1), |_args: &[String]| -> Result<(), String> { Ok(()) }, |args: &Vec, _ctx: &CmdContext| -> Vec> { let path = args.first().map(|s| s.as_str()).unwrap_or("export.csv"); @@ -437,6 +574,7 @@ effect_cmd!( effect_cmd!( WriteCmd, "w", + MaxArgs::AtMost(1), |_args: &[String]| -> Result<(), String> { Ok(()) }, |args: &Vec, _ctx: &CmdContext| -> Vec> { if args.is_empty() { @@ -450,6 +588,7 @@ effect_cmd!( effect_cmd!( HelpCmd, "help", + MaxArgs::AtMost(0), |_args: &[String]| -> Result<(), String> { Ok(()) }, |_args: &Vec, _ctx: &CmdContext| -> Vec> { vec![ @@ -462,6 +601,7 @@ effect_cmd!( effect_cmd!( HelpPageNextCmd, "help-page-next", + MaxArgs::AtMost(0), |_args: &[String]| -> Result<(), String> { Ok(()) }, |_args: &Vec, _ctx: &CmdContext| -> Vec> { vec![effect::help_page_next()] @@ -471,6 +611,7 @@ effect_cmd!( effect_cmd!( HelpPagePrevCmd, "help-page-prev", + MaxArgs::AtMost(0), |_args: &[String]| -> Result<(), String> { Ok(()) }, |_args: &Vec, _ctx: &CmdContext| -> Vec> { vec![effect::help_page_prev()] @@ -480,6 +621,7 @@ effect_cmd!( effect_cmd!( LoadModelCmd, "load", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::LoadModel(std::path::PathBuf::from( @@ -491,6 +633,7 @@ effect_cmd!( effect_cmd!( ExportCsvCmd, "export-csv", + MaxArgs::AtMost(1), |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::ExportCsv(std::path::PathBuf::from( @@ -502,6 +645,7 @@ effect_cmd!( effect_cmd!( ImportJsonCmd, "import-json", + MaxArgs::AtMost(3), |args: &[String]| { if args.is_empty() { Err("import-json requires a path".to_string()) diff --git a/crates/improvise-command/src/command/cmd/fit.rs b/crates/improvise-command/src/command/cmd/fit.rs index da7fb3c3..8c2fb087 100644 --- a/crates/improvise-command/src/command/cmd/fit.rs +++ b/crates/improvise-command/src/command/cmd/fit.rs @@ -4,7 +4,7 @@ use improvise_gp::fit::one_line; use improvise_gp::{FitRequest, fit_measure, fitted_measure_name}; -use crate::command::cmd::core::{Cmd, CmdContext}; +use crate::command::cmd::core::{Cmd, CmdContext, MaxArgs}; use crate::ui::effect::{self, Effect}; /// A seed as the status line spells it (`0x…`, either case) or in decimal. The hex @@ -27,6 +27,8 @@ pub struct FitMeasure { } impl FitMeasure { + pub const MAX_ARGS: MaxArgs = MaxArgs::AtMost(3); + /// A nameless instance for `register_pure`, which reads a command's name /// off a prototype rather than off a string literal (gotcha 15). pub fn prototype() -> Self { diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index e394c118..e7bb964c 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -81,52 +81,130 @@ pub fn default_registry() -> CmdRegistry { let mut r = CmdRegistry::new(); // ── Model mutations (effect_cmd! wrappers) ─────────────────────────── - r.register_pure(&AddCategoryCmd(vec![]), AddCategoryCmd::parse); - r.register_pure(&AddItemCmd(vec![]), AddItemCmd::parse); - r.register_pure(&AddItemsCmd(vec![]), AddItemsCmd::parse); - r.register_pure(&AddItemInGroupCmd(vec![]), AddItemInGroupCmd::parse); - r.register_pure(&SetCellCmd(vec![]), SetCellCmd::parse); - r.register_pure(&ClearBufferCmd(vec![]), ClearBufferCmd::parse); + r.register_pure( + &AddCategoryCmd(vec![]), + AddCategoryCmd::MAX_ARGS, + AddCategoryCmd::parse, + ); + r.register_pure(&AddItemCmd(vec![]), AddItemCmd::MAX_ARGS, AddItemCmd::parse); + r.register_pure( + &AddItemsCmd(vec![]), + AddItemsCmd::MAX_ARGS, + AddItemsCmd::parse, + ); + r.register_pure( + &AddItemInGroupCmd(vec![]), + AddItemInGroupCmd::MAX_ARGS, + AddItemInGroupCmd::parse, + ); + r.register_pure(&SetCellCmd(vec![]), SetCellCmd::MAX_ARGS, SetCellCmd::parse); + r.register_pure( + &ClearBufferCmd(vec![]), + ClearBufferCmd::MAX_ARGS, + ClearBufferCmd::parse, + ); r.register( &ClearCellCommand { key: CellKey::new(vec![]), }, + MaxArgs::Any, |args| { if args.is_empty() { return Err("clear-cell requires at least one Cat/Item coordinate".into()); } Ok(ClearCellCommand { - key: parse_cell_key_from_args(args), + key: parse_cell_key_from_args(args)?, }) }, - Some(|_, ctx| { + Some(|args, ctx| { + only_when_omitted(args)?; Ok(ClearCellCommand { key: cursor_value_key(ctx)?, }) }), ); - r.register_pure(&AddFormulaCmd(vec![]), AddFormulaCmd::parse); - r.register_pure(&FitMeasure::prototype(), FitMeasure::parse); - r.register_pure(&RemoveFormulaCmd(vec![]), RemoveFormulaCmd::parse); - r.register_pure(&CreateViewCmd(vec![]), CreateViewCmd::parse); - r.register_pure(&DeleteViewCmd(vec![]), DeleteViewCmd::parse); - r.register_pure(&SwitchViewCmd(vec![]), SwitchViewCmd::parse); - r.register_pure(&SetAxisCmd(vec![]), SetAxisCmd::parse); - r.register_pure(&SetPageCmd(vec![]), SetPageCmd::parse); - r.register_pure(&ToggleGroupCmd(vec![]), ToggleGroupCmd::parse); - r.register_pure(&HideItemCmd(vec![]), HideItemCmd::parse); - r.register_pure(&ShowItemCmd(vec![]), ShowItemCmd::parse); - r.register_pure(&SaveAsCmd(vec![]), SaveAsCmd::parse); - r.register_pure(&LoadModelCmd(vec![]), LoadModelCmd::parse); - r.register_pure(&ExportCsvCmd(vec![]), ExportCsvCmd::parse); - r.register_pure(&ImportJsonCmd(vec![]), ImportJsonCmd::parse); - r.register_pure(&SetFormatCmd(vec![]), SetFormatCmd::parse); - r.register_pure(&ImportCmd(vec![]), ImportCmd::parse); - r.register_pure(&ExportCmd(vec![]), ExportCmd::parse); - r.register_pure(&WriteCmd(vec![]), WriteCmd::parse); - r.register_pure(&HelpCmd(vec![]), HelpCmd::parse); - r.register_pure(&HelpPageNextCmd(vec![]), HelpPageNextCmd::parse); - r.register_pure(&HelpPagePrevCmd(vec![]), HelpPagePrevCmd::parse); + r.register_pure( + &AddFormulaCmd(vec![]), + AddFormulaCmd::MAX_ARGS, + AddFormulaCmd::parse, + ); + r.register_pure( + &FitMeasure::prototype(), + FitMeasure::MAX_ARGS, + FitMeasure::parse, + ); + r.register_pure( + &RemoveFormulaCmd(vec![]), + RemoveFormulaCmd::MAX_ARGS, + RemoveFormulaCmd::parse, + ); + r.register_pure( + &CreateViewCmd(vec![]), + CreateViewCmd::MAX_ARGS, + CreateViewCmd::parse, + ); + r.register_pure( + &DeleteViewCmd(vec![]), + DeleteViewCmd::MAX_ARGS, + DeleteViewCmd::parse, + ); + r.register_pure( + &SwitchViewCmd(vec![]), + SwitchViewCmd::MAX_ARGS, + SwitchViewCmd::parse, + ); + r.register_pure(&SetAxisCmd(vec![]), SetAxisCmd::MAX_ARGS, SetAxisCmd::parse); + r.register_pure(&SetPageCmd(vec![]), SetPageCmd::MAX_ARGS, SetPageCmd::parse); + r.register_pure( + &ToggleGroupCmd(vec![]), + ToggleGroupCmd::MAX_ARGS, + ToggleGroupCmd::parse, + ); + r.register_pure( + &HideItemCmd(vec![]), + HideItemCmd::MAX_ARGS, + HideItemCmd::parse, + ); + r.register_pure( + &ShowItemCmd(vec![]), + ShowItemCmd::MAX_ARGS, + ShowItemCmd::parse, + ); + r.register_pure(&SaveAsCmd(vec![]), SaveAsCmd::MAX_ARGS, SaveAsCmd::parse); + r.register_pure( + &LoadModelCmd(vec![]), + LoadModelCmd::MAX_ARGS, + LoadModelCmd::parse, + ); + r.register_pure( + &ExportCsvCmd(vec![]), + ExportCsvCmd::MAX_ARGS, + ExportCsvCmd::parse, + ); + r.register_pure( + &ImportJsonCmd(vec![]), + ImportJsonCmd::MAX_ARGS, + ImportJsonCmd::parse, + ); + r.register_pure( + &SetFormatCmd(vec![]), + SetFormatCmd::MAX_ARGS, + SetFormatCmd::parse, + ); + r.register_pure(&ImportCmd(vec![]), ImportCmd::MAX_ARGS, ImportCmd::parse); + r.register_pure(&ExportCmd(vec![]), ExportCmd::MAX_ARGS, ExportCmd::parse); + r.register_pure(&WriteCmd(vec![]), WriteCmd::MAX_ARGS, WriteCmd::parse); + r.register_pure(&HelpCmd(vec![]), HelpCmd::MAX_ARGS, HelpCmd::parse); + r.register_pure( + &HelpPageNextCmd(vec![]), + HelpPageNextCmd::MAX_ARGS, + HelpPageNextCmd::parse, + ); + r.register_pure( + &HelpPagePrevCmd(vec![]), + HelpPagePrevCmd::MAX_ARGS, + HelpPagePrevCmd::parse, + ); // ── Navigation (unified Move) ────────────────────────────────────── r.register_at_cursor( @@ -135,6 +213,7 @@ pub fn default_registry() -> CmdRegistry { cursor: CursorState::default(), cmd_name: "move-selection", }, + MaxArgs::AtMost(2), |args| { require_args(args, 2)?; let dr = args[0].parse::().map_err(|e| e.to_string())?; @@ -160,6 +239,7 @@ pub fn default_registry() -> CmdRegistry { cursor: CursorState::default(), cmd_name: $name, }, + MaxArgs::AtMost(0), |_| { Ok(Move { kind: if $to_end { @@ -185,6 +265,7 @@ pub fn default_registry() -> CmdRegistry { cursor: CursorState::default(), cmd_name: "scroll-rows", }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let n = args[0].parse::().map_err(|e| e.to_string())?; @@ -202,6 +283,7 @@ pub fn default_registry() -> CmdRegistry { cursor: CursorState::default(), cmd_name: "page-scroll", }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let dir = args[0].parse::().map_err(|e| e.to_string())?; @@ -217,6 +299,7 @@ pub fn default_registry() -> CmdRegistry { &EnterAdvance { cursor: CursorState::default(), }, + MaxArgs::AtMost(0), |_| { Ok(EnterAdvance { cursor: CursorState { @@ -239,15 +322,17 @@ pub fn default_registry() -> CmdRegistry { &YankCell { key: CellKey::new(vec![]), }, + MaxArgs::Any, |args| { if args.is_empty() { return Err("yank requires at least one Cat/Item coordinate".into()); } Ok(YankCell { - key: parse_cell_key_from_args(args), + key: parse_cell_key_from_args(args)?, }) }, - Some(|_, ctx| { + Some(|args, ctx| { + only_when_omitted(args)?; Ok(YankCell { key: cursor_key_to_read(ctx)?, }) @@ -257,15 +342,17 @@ pub fn default_registry() -> CmdRegistry { &PasteCell { at: PasteAt::Spelled(CellKey::new(vec![])), }, + MaxArgs::Any, |args| { if args.is_empty() { return Err("paste requires at least one Cat/Item coordinate".into()); } Ok(PasteCell { - at: PasteAt::Spelled(parse_cell_key_from_args(args)), + at: PasteAt::Spelled(parse_cell_key_from_args(args)?), }) }, - Some(|_, ctx| { + Some(|args, ctx| { + only_when_omitted(args)?; Ok(PasteCell { at: PasteAt::Resolved(cursor_value_key(ctx)?), }) @@ -288,6 +375,7 @@ pub fn default_registry() -> CmdRegistry { &EditOrDrill { edit_mode: AppMode::Normal, }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let edit_mode = parse_mode_name(&args[0])?; @@ -298,6 +386,7 @@ pub fn default_registry() -> CmdRegistry { &EnterEditAtCursorCmd { target_mode: AppMode::Normal, }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let target_mode = parse_mode_name(&args[0])?; @@ -309,15 +398,17 @@ pub fn default_registry() -> CmdRegistry { &DrillIntoCell { key: crate::model::cell::CellKey::new(vec![]), }, + MaxArgs::Any, |args| { if args.is_empty() { return Err("drill-into-cell requires Cat/Item coordinates".into()); } Ok(DrillIntoCell { - key: parse_cell_key_from_args(args), + key: parse_cell_key_from_args(args)?, }) }, - Some(|_, ctx| { + Some(|args, ctx| { + only_when_omitted(args)?; Ok(DrillIntoCell { key: cursor_key(ctx)?, }) @@ -325,13 +416,13 @@ pub fn default_registry() -> CmdRegistry { ); r.register_nullary(|| Box::new(ViewNavigate { forward: false })); r.register_nullary(|| Box::new(ViewNavigate { forward: true })); - r.register_pure(&EnterMode(AppMode::Normal), |args| { + r.register_pure(&EnterMode(AppMode::Normal), MaxArgs::AtMost(1), |args| { require_args(args, 1)?; Ok(Box::new(EnterMode(parse_mode_name(&args[0])?))) }); // ── Search ─────────────────────────────────────────────────────────── - r.register_pure(&SearchNavigate(true), |args| { + r.register_pure(&SearchNavigate(true), MaxArgs::AtMost(1), |args| { let forward = args.first().map(|s| s != "backward").unwrap_or(true); Ok(Box::new(SearchNavigate(forward))) }); @@ -344,6 +435,7 @@ pub fn default_registry() -> CmdRegistry { panel: Panel::Formula, target: None, }, + MaxArgs::AtMost(3), |args| { // Parse: toggle-panel-and-focus [open] [focused] require_args(args, 1)?; @@ -360,6 +452,7 @@ pub fn default_registry() -> CmdRegistry { &TogglePanelVisibility { panel: Panel::Formula, }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let panel = parse_panel(&args[0])?; @@ -373,6 +466,7 @@ pub fn default_registry() -> CmdRegistry { panel: Panel::Formula, delta: 0, }, + MaxArgs::AtMost(2), |args| { require_args(args, 2)?; let panel = parse_panel(&args[0])?; @@ -396,7 +490,7 @@ pub fn default_registry() -> CmdRegistry { r.register_nullary(|| Box::new(DeleteViewAtCursor)); // ── Tile select ────────────────────────────────────────────────────── - r.register_pure(&MoveTileCursor(0), |args| { + r.register_pure(&MoveTileCursor(0), MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let delta = args[0].parse::().map_err(|e| e.to_string())?; Ok(Box::new(MoveTileCursor(delta))) @@ -406,6 +500,7 @@ pub fn default_registry() -> CmdRegistry { &TileAxisOp { axis: Some(crate::view::Axis::Row), }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let axis = parse_axis(&args[0])?; @@ -423,6 +518,7 @@ pub fn default_registry() -> CmdRegistry { &AppendChar { buffer: String::new(), }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; Ok(Box::new(AppendChar { @@ -434,6 +530,7 @@ pub fn default_registry() -> CmdRegistry { &PopChar { buffer: String::new(), }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; Ok(Box::new(PopChar { @@ -454,13 +551,14 @@ pub fn default_registry() -> CmdRegistry { edit_mode: AppMode::editing(), opened: None, }, + MaxArgs::Any, |args| { if args.len() < 3 { return Err("commit-cell-edit requires a mode, value, and coords".into()); } let edit_mode = parse_mode_name(&args[0])?; Ok(CommitAndAdvance { - key: parse_cell_key_from_args(&args[2..]), + key: parse_cell_key_from_args(&args[2..])?, value: args[1].clone(), advance: AdvanceDir::Down, cursor: CursorState::default(), @@ -479,6 +577,7 @@ pub fn default_registry() -> CmdRegistry { edit_mode: AppMode::editing(), opened: None, }, + MaxArgs::AtMost(1), |_| Err("commit-and-advance-right requires context".into()), Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); @@ -486,6 +585,7 @@ pub fn default_registry() -> CmdRegistry { &CommitBuffer { kind: CommitKind::Category, }, + MaxArgs::AtMost(1), |args| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; @@ -534,6 +634,7 @@ pub(super) mod tests { vec!["a"], vec!["Region"], vec!["Region", "East"], + vec!["Region", "row"], vec!["Region", "East", "1"], vec!["Profit = 1"], vec![",.2f"], @@ -544,6 +645,7 @@ pub(super) mod tests { vec!["editing", "5", "Region/East"], vec!["formula", "false", "false"], vec!["Region/East"], + vec!["1", "Region/East"], ] .into_iter() .map(|v| v.into_iter().map(String::from).collect()) @@ -592,6 +694,87 @@ pub(super) mod tests { ); } + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn no_command_silently_drops_an_argument_it_was_given() { + let r = default_registry(); + let wb = two_cat_model(); + let layout = make_layout(&wb); + let reg = make_registry(); + let ctx = make_ctx(&wb, &layout, ®); + let variadic = [ + "add-items", + "set-cell", + "set-format", + "clear-cell", + "yank", + "paste", + "drill-into-cell", + "commit-cell-edit", + ]; + // Their own refusal states the shape, and it runs before the rule. + let own_shape = ["add-formula", "fit-measure", "commit-and-advance-right"]; + let mut checked = 0; + for name in r.names().collect::>() { + if variadic.contains(&name) { + continue; + } + for args in candidate_args() { + // Four more than any list accepted, so an optional argument + // cannot absorb them; the last one is surplus whatever the command. + let mut more = args.clone(); + more.extend(["EXTRA1", "EXTRA2", "EXTRA3", "EXTRA4"].map(String::from)); + let arms = [ + r.parse(name, &args).map(|_| r.parse(name, &more)), + r.interactive(name, &args, &ctx) + .map(|_| r.interactive(name, &more, &ctx)), + ]; + for extended in arms.into_iter().flatten() { + checked += 1; + match extended { + Ok(_) => panic!("`{name} {more:?}` dropped arguments without a word"), + Err(e) if own_shape.contains(&name) || e.contains("\"EXTRA4\"") => {} + Err(e) => { + panic!("`{name}` refused an extra argument without naming it: {e}") + } + } + } + } + } + assert!(checked > 100, "only {checked} accepted lists were extended"); + } + + /// Codex on PR #316: a variadic coordinate list dropped a token that was no + /// coordinate. Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_malformed_coordinate_is_refused_not_dropped() { + let lines = [ + "set-cell 1 Type/Food unexpected", + "clear-cell Type/Food unexpected", + "yank Type/Food unexpected", + "paste Type/Food unexpected", + "drill-into-cell Type/Food unexpected", + "commit-cell-edit editing 5 Type/Food unexpected", + ]; + for line in lines { + let err = crate::command::parse_line(line) + .err() + .unwrap_or_else(|| panic!("`{line}` dropped \"unexpected\" without a word")); + assert!( + err.contains("\"unexpected\""), + "`{line}` did not name it: {err}" + ); + } + for half_empty in ["set-cell 1 Type/", "clear-cell /Food"] { + assert!( + crate::command::parse_line(half_empty).is_err(), + "`{half_empty}`" + ); + } + let control = "set-cell 1 Type/Food Month/Jan . clear-cell Type/Food"; + assert!(crate::command::parse_line(control).is_ok(), "the control"); + } + /// A variant mix-up shows up as a **collision**, not a mismatch: the wrong entry /// takes its sibling's name and the sibling vanishes. Uniqueness catches it. #[test] diff --git a/crates/improvise-command/src/command/cmd/text_buffer.rs b/crates/improvise-command/src/command/cmd/text_buffer.rs index 300b1336..50fc5ea3 100644 --- a/crates/improvise-command/src/command/cmd/text_buffer.rs +++ b/crates/improvise-command/src/command/cmd/text_buffer.rs @@ -216,6 +216,110 @@ mod tests { app.apply_effects(effects); } + fn cell(wb: &crate::workbook::Workbook, ty: &str, month: &str) -> Option { + use crate::model::cell::{CellKey, CellValue}; + let key = CellKey::new(vec![ + ("Type".into(), ty.into()), + ("Month".into(), month.into()), + ]); + match wb.model.get_cell(&key) { + Some(CellValue::Number(n)) => Some(*n), + _ => None, + } + } + + /// An app whose cursor (0,0) is on Food/Jan = 1, with Clothing/Feb = 2 beside it. + fn app_with_two_cells() -> crate::ui::app::App { + use crate::model::cell::{CellKey, CellValue}; + let mut app = wizard_test_app(); + for (ty, month, n) in [("Food", "Jan", 1.0), ("Clothing", "Feb", 2.0)] { + let key = CellKey::new(vec![ + ("Type".into(), ty.into()), + ("Month".into(), month.into()), + ]); + app.model_state + .workbook + .model + .set_cell(key, CellValue::Number(n)); + } + app + } + + /// PR #316 review: a malformed coordinate on `:` fell into the cursor fallback + /// and wrote the CURSOR's cell. Evidence: `context/test-evidence/cli.md`. + #[test] + fn colon_refuses_a_malformed_coordinate_and_leaves_the_cursor_cell_alone() { + let lines = [ + "clear-cell Type/Clothing Month/Feb junk", + "yank Type/Clothing Month/Feb junk", + "paste Type/Clothing Month/Feb junk", + "drill-into-cell Type/Clothing Month/Feb junk", + ]; + for line in lines { + let mut app = app_with_two_cells(); + app.view_state.yanked = Some(crate::model::cell::CellValue::Number(9.0)); + let before = app.view_state.yanked.clone(); + run_colon_command(&mut app, line); + assert!(app.last_batch_refused(), "`:{line}` was not refused"); + assert!( + app.view_state.status_msg.contains("\"junk\""), + "`:{line}` did not name the token: {:?}", + app.view_state.status_msg + ); + let wb = &app.model_state.workbook; + assert_eq!( + cell(wb, "Food", "Jan"), + Some(1.0), + "`:{line}` wrote the cursor cell" + ); + assert_eq!( + cell(wb, "Clothing", "Feb"), + Some(2.0), + "`:{line}` wrote the named cell" + ); + assert_eq!(app.view_state.yanked, before, "`:{line}` yanked"); + assert!(app.drill_state.is_none(), "`:{line}` opened a drill"); + } + } + + /// The fallback still answers for coordinates that were OMITTED. + #[test] + fn colon_clear_cell_with_no_coordinates_still_uses_the_cursor() { + let mut app = app_with_two_cells(); + run_colon_command(&mut app, "clear-cell"); + let wb = &app.model_state.workbook; + assert_eq!(cell(wb, "Food", "Jan"), None); + assert_eq!(cell(wb, "Clothing", "Feb"), Some(2.0)); + } + + /// A later `:` segment sees what an earlier one made, as `improvise cmd` does: + /// the line is built before either half runs. Evidence: `context/test-evidence/cli.md`. + #[test] + fn colon_lets_a_later_segment_use_what_an_earlier_one_created() { + let mut app = wizard_test_app(); + run_colon_command(&mut app, "create-view Second . switch-view Second"); + assert!(!app.last_batch_refused(), "{:?}", app.view_state.status_msg); + assert_eq!(app.view_state.active_view, "Second"); + + let mut app = wizard_test_app(); + run_colon_command(&mut app, "add-item Type Tools . set-page Type Tools"); + assert!(!app.last_batch_refused(), "{:?}", app.view_state.status_msg); + } + + /// The control: a name NOTHING creates is still refused, on `:` too. + #[test] + fn colon_still_refuses_a_name_nothing_creates() { + for line in [ + "switch-view Nowhere", + "set-page Type Nowhere", + "hide-item Regoin Food", + ] { + let mut app = wizard_test_app(); + run_colon_command(&mut app, line); + assert!(app.last_batch_refused(), "`:{line}` was not refused"); + } + } + /// improvise-toc: `StartImportWizard` swaps the mode only on success, so a /// per-type answer snapped `:import` straight back out of the wizard. #[test] @@ -271,6 +375,43 @@ mod tests { ); } + /// improvise-31oe.2 on the `:` line: the refusal is on the status line, + /// naming what is missing. Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_colon_line_naming_something_missing_refuses_on_the_status_line() { + for (line, missing) in [ + ("set-axis Regoin row", "Regoin"), + ("set-axis Type sideways", "sideways"), + ("set-page Type Nowhere", "Nowhere"), + ("create-view By area", "\"area\""), + ] { + let mut app = wizard_test_app(); + run_colon_command(&mut app, line); + let said = &app.view_state.status_msg; + assert!( + app.last_batch_refused(), + "`:{line}` was not refused: {said:?}" + ); + assert!( + said.contains(missing), + "`:{line}` did not name {missing}: {said:?}" + ); + assert!(matches!(app.view_state.mode, AppMode::Normal), "`:{line}`"); + } + } + + /// improvise-31oe.3 on the `:` line. + #[test] + fn a_colon_line_reads_a_pipe_quoted_name() { + let mut app = wizard_test_app(); + run_colon_command(&mut app, "create-view |By area|"); + let views: Vec<&String> = app.model_state.workbook.views.keys().collect(); + assert!( + app.model_state.workbook.views.contains_key("By area"), + "{views:?}" + ); + } + /// A command that does not touch the mode still returns to Normal. #[test] fn colon_plain_command_returns_to_normal() { @@ -372,9 +513,12 @@ impl Cmd for ExecuteCommand { // Built against the live cursor, exactly as a script's is — improvise-6uva. let built: Result>, String> = crate::command::parse::segments(&raw) - .into_iter() - .map(|seg| crate::command::parse::parse_segment_interactive(seg, ctx)) - .collect::, _>>() + .and_then(|segments| { + segments + .into_iter() + .map(|seg| crate::command::parse::parse_segment_interactive(seg, ctx)) + .collect::, _>>() + }) .map(|built| built.into_iter().flatten().collect()); match built { Ok(cmds) => { diff --git a/crates/improvise-command/src/command/parse.rs b/crates/improvise-command/src/command/parse.rs index 3f236d47..6f68a90b 100644 --- a/crates/improvise-command/src/command/parse.rs +++ b/crates/improvise-command/src/command/parse.rs @@ -1,5 +1,10 @@ //! Quasi-lisp prefix command parser: `word arg1 arg2 …`, `.` between commands, -//! `Category/Item` coordinate pairs, `"quoted strings"`. +//! `Category/Item` coordinate pairs, `"quoted strings"` and `|pipe-quoted names|`. + +use std::iter::Peekable; +use std::str::CharIndices; + +use improvise_formula::quote::unescape_pipe_body; use super::cmd::{Cmd, CmdContext, CmdRegistry, default_registry}; @@ -11,39 +16,19 @@ pub fn parse_line(line: &str) -> Result>, String> { /// Parse a line into commands using a given registry. pub fn parse_line_with(registry: &CmdRegistry, line: &str) -> Result>, String> { - let line = line.trim(); - if line.is_empty() || line.starts_with('#') || line.starts_with("//") { - return Ok(vec![]); - } - - let mut commands = Vec::new(); - for segment in split_on_dot(line) { - let segment = segment.trim(); - if segment.is_empty() { - continue; - } - let tokens = tokenize(segment); - if tokens.is_empty() { - continue; - } - let word = &tokens[0]; - let args = &tokens[1..]; - commands.push(registry.parse(word, args)?); - } - Ok(commands) + groups(line.trim())? + .iter() + .map(|words| registry.parse(&words[0].text, &texts(&words[1..]))) + .collect() } /// The command segments of a line, with comments and blank segments dropped. -pub fn segments(line: &str) -> Vec<&str> { +pub fn segments(line: &str) -> Result, String> { let line = line.trim(); - if line.is_empty() || line.starts_with('#') || line.starts_with("//") { - return Vec::new(); - } - split_on_dot(line) - .into_iter() - .map(str::trim) - .filter(|s| !s.is_empty()) - .collect() + Ok(groups(line)? + .iter() + .map(|words| &line[words[0].start..words[words.len() - 1].end]) + .collect()) } /// Build one segment's command against a LIVE context — `context/code/cli.md`. @@ -51,7 +36,7 @@ pub fn parse_segment_interactive( segment: &str, ctx: &CmdContext, ) -> Result>, String> { - let tokens = tokenize(segment.trim()); + let tokens = texts(&lex(segment.trim())?); let Some(word) = tokens.first() else { return Ok(None); }; @@ -67,65 +52,99 @@ pub fn parse_segment_interactive( }) } -/// Split a line on ` . ` separators (dot must be a standalone word, -/// surrounded by whitespace or at line boundaries). Respects quoted strings. -fn split_on_dot(line: &str) -> Vec<&str> { - let mut segments = Vec::new(); - let mut start = 0; - let mut in_quote = false; - let bytes = line.as_bytes(); - - for (i, c) in line.char_indices() { - match c { - '"' => in_quote = !in_quote, - '.' if !in_quote => { - let before_ws = i == 0 || bytes[i - 1].is_ascii_whitespace(); - let after_ws = i + 1 >= bytes.len() || bytes[i + 1].is_ascii_whitespace(); - if before_ws && after_ws { - segments.push(&line[start..i]); - start = i + 1; - } - } - _ => {} - } - } - segments.push(&line[start..]); - segments +/// One word of a line and the byte span it was read from. A `separator` is a +/// bare `.`; a quoted one is an argument like any other. +struct Word { + text: String, + start: usize, + end: usize, + separator: bool, } -/// Tokenize a command segment into words, handling quoted strings. -fn tokenize(input: &str) -> Vec { - let mut tokens = Vec::new(); - let mut chars = input.chars().peekable(); +fn texts(words: &[Word]) -> Vec { + words.iter().map(|w| w.text.clone()).collect() +} - while let Some(&c) = chars.peek() { +/// The words of a trimmed line, split at separators, blank commands and +/// comment lines dropped. +fn groups(line: &str) -> Result>, String> { + if line.starts_with('#') || line.starts_with("//") { + return Ok(Vec::new()); + } + let mut groups: Vec> = vec![Vec::new()]; + for word in lex(line)? { + match word.separator { + true => groups.push(Vec::new()), + false => groups.last_mut().expect("never empty").push(word), + } + } + groups.retain(|g| !g.is_empty()); + Ok(groups) +} + +/// Split `line` into words. The quoting rules: `context/code/cli.md` §14. +fn lex(line: &str) -> Result, String> { + let mut words = Vec::new(); + let mut chars = line.char_indices().peekable(); + while let Some(&(start, c)) = chars.peek() { if c.is_whitespace() { chars.next(); continue; } - if c == '"' { - chars.next(); // consume opening quote - let mut s = String::new(); - for ch in chars.by_ref() { - if ch == '"' { - break; - } - s.push(ch); + let (text, bare) = match c { + '"' => (double_quoted(&mut chars), false), + _ => bare_word(&mut chars, line)?, + }; + let end = chars.peek().map_or(line.len(), |&(i, _)| i); + words.push(Word { + separator: bare && text == ".", + text, + start, + end, + }); + } + Ok(words) +} + +/// `"…"` read verbatim: no escapes, and a `|` inside stays a `|`. +fn double_quoted(chars: &mut Peekable) -> String { + chars.next(); + chars + .by_ref() + .map(|(_, c)| c) + .take_while(|&c| c != '"') + .collect() +} + +/// Text up to whitespace, each `|…|` piece unquoted in place; `bare` is false +/// once a piece was quoted, so a quoted `|.|` is an argument and not a separator. +fn bare_word(chars: &mut Peekable, line: &str) -> Result<(String, bool), String> { + let mut text = String::new(); + let mut bare = true; + while let Some((at, c)) = chars.next_if(|&(_, c)| !c.is_whitespace()) { + match c { + '|' => { + text.push_str(&pipe_body(chars, line, at)?); + bare = false; } - tokens.push(s); - } else { - let mut s = String::new(); - while let Some(&ch) = chars.peek() { - if ch.is_whitespace() { - break; - } - s.push(ch); - chars.next(); - } - tokens.push(s); + _ => text.push(c), } } - tokens + Ok((text, bare)) +} + +/// The body of the `|…|` opened at `open`, escapes applied by the one shared +/// reader (gotcha 34). +fn pipe_body(chars: &mut Peekable, line: &str, open: usize) -> Result { + let mut body = String::new(); + while let Some((_, c)) = chars.next() { + match c { + '|' => return Ok(unescape_pipe_body(&body)), + '\\' => body.extend(std::iter::once(c).chain(chars.next().map(|(_, e)| e))), + _ => body.push(c), + } + } + Err(format!("unterminated |…| in `{}`", &line[open..])) } #[cfg(test)] @@ -302,11 +321,14 @@ mod tests { #[test] fn segments_drops_comments_and_blanks() { - assert_eq!(segments("# a comment"), Vec::<&str>::new()); - assert_eq!(segments("// also a comment"), Vec::<&str>::new()); - assert_eq!(segments(" "), Vec::<&str>::new()); - assert_eq!(segments("a . . b"), vec!["a", "b"]); - assert_eq!(segments("move-selection 2 0"), vec!["move-selection 2 0"]); + assert_eq!(segments("# a comment"), Ok(Vec::<&str>::new())); + assert_eq!(segments("// also a comment"), Ok(Vec::<&str>::new())); + assert_eq!(segments(" "), Ok(Vec::<&str>::new())); + assert_eq!(segments("a . . b"), Ok(vec!["a", "b"])); + assert_eq!( + segments("move-selection 2 0"), + Ok(vec!["move-selection 2 0"]) + ); } /// Evidence: `context/test-evidence/cli.md`. @@ -347,4 +369,105 @@ mod tests { .expect_err("an unknown word is refused"); assert!(err.contains("no-such-command"), "got: {err}"); } + + // ── pipe quoting and arity (improvise-31oe.3) ───────────────────────── + + /// The one command `line` builds, as its `Debug` shows it. + fn built(line: &str) -> String { + let cmds = parse_line(line).unwrap_or_else(|e| panic!("`{line}` must parse: {e}")); + assert_eq!(cmds.len(), 1, "`{line}` is one command"); + format!("{:?}", cmds[0]) + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_pipe_quoted_argument_is_one_argument_without_its_pipes() { + assert_eq!( + built("create-view |By area|"), + r#"CreateViewCmd(["By area"])"# + ); + assert_eq!( + built("set-page |Sales Region| |East Coast|"), + r#"SetPageCmd(["Sales Region", "East Coast"])"# + ); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_pipe_quoted_argument_reads_the_escapes_improv_does() { + assert_eq!(built(r"create-view |a\|b|"), r#"CreateViewCmd(["a|b"])"#); + assert_eq!( + built(r"create-view |back\\slash|"), + r#"CreateViewCmd(["back\\slash"])"# + ); + assert_eq!( + built(r"create-view |two\nlines|"), + r#"CreateViewCmd(["two\nlines"])"# + ); + } + + /// A coordinate quotes each half, as a `.improv` data line does. + #[test] + fn a_coordinate_may_pipe_quote_either_half() { + assert_eq!( + built("set-cell 5 |Sales Region|/|East Coast| Measure/Revenue"), + r#"SetCellCmd(["5", "Sales Region/East Coast", "Measure/Revenue"])"# + ); + } + + #[test] + fn a_dot_inside_pipes_does_not_split_the_line() { + assert_eq!(built("create-view |a . b|"), r#"CreateViewCmd(["a . b"])"#); + assert_eq!( + segments("create-view |a . b|"), + Ok(vec!["create-view |a . b|"]) + ); + } + + /// Double quotes stay verbatim: formula text carries its own pipes. + #[test] + fn a_double_quoted_argument_keeps_the_pipes_inside_it() { + assert_eq!( + built(r#"add-formula Measure "Net = |Gross Rev| - Cost""#), + r#"AddFormulaCmd(["Measure", "Net = |Gross Rev| - Cost"])"# + ); + } + + /// `context/code/cli.md` §14: only a BARE `.` separates commands. A quoted one + /// is an argument, so `|.|` and `"."` name a thing called `.`. + #[test] + fn a_quoted_dot_is_an_argument_not_a_separator() { + for line in ["create-view |.|", "create-view \".\""] { + let groups = groups(line).expect("lexes"); + assert_eq!(groups.len(), 1, "`{line}` split into commands"); + assert_eq!(texts(&groups[0]), ["create-view", "."], "`{line}`"); + } + assert_eq!(groups("create-view A . create-view B").unwrap().len(), 2); + } + + #[test] + fn an_unterminated_pipe_is_refused() { + let err = parse_line("create-view |By area").expect_err("an open pipe is refused"); + assert!( + err.contains("|By area"), + "the refusal names the text: {err}" + ); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_command_given_more_arguments_than_it_takes_is_refused() { + let err = parse_line("create-view By area").expect_err("two words for one argument"); + assert!(err.starts_with("create-view:"), "names the command: {err}"); + assert!(err.contains("\"area\""), "names the extra argument: {err}"); + let err = parse_line("set-axis Type row sideways").expect_err("a third argument"); + assert!( + err.contains("\"sideways\""), + "names the extra argument: {err}" + ); + assert!( + parse_line("create-view |By area|").is_ok(), + "the control parses" + ); + } } diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index e28d8c87..47425e2b 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -3462,7 +3462,9 @@ mod tests { /// when any segment refused, which is what its exit code reads. fn run_line(app: &mut App, line: &str) -> bool { let mut refused = false; - for segment in crate::command::parse::segments(line) { + let segments = crate::command::parse::segments(line) + .unwrap_or_else(|e| panic!("`{line}` must lex: {e}")); + for segment in segments { let effects = { let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); crate::command::parse::parse_segment_interactive(segment, &ctx) diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 71addd5f..b7d5883b 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1368,6 +1368,63 @@ impl Effect for SetPanelCursor { // ── Chain control ──────────────────────────────────────────────────────────── +/// What a command's effects name that the document must have. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Needs { + Category(String), + Item(String, String), + Group(String, String), + View(String), +} + +/// Why the document cannot satisfy `needs` in the active view, if it cannot. +fn unknown_name(app: &App, needs: &Needs) -> Option { + let workbook = &app.model_state.workbook; + let (category, rest) = match needs { + Needs::View(name) => { + return (!workbook.views.contains_key(name)) + .then(|| format!("No view named \"{name}\"")); + } + Needs::Category(c) => (c, None), + Needs::Item(c, i) => (c, Some((i, false))), + Needs::Group(c, g) => (c, Some((g, true))), + }; + let view = workbook.views.get(app.view_name())?; + if view.try_axis_of(category).is_none() { + return Some(format!("Unknown category '{category}'")); + } + let (name, is_group) = rest?; + if !is_group { + // `effective_item_names`, so a formula target is a known `_Measure` item. + let known = workbook + .model + .effective_item_names(category) + .iter() + .any(|i| i == name); + return (!known).then(|| format!("Unknown item '{name}' in '{category}'")); + } + // An item's own group registers no `Group`; a parent group has no items. + let known = workbook.model.category(category).is_some_and(|c| { + !c.items_in_group(name).is_empty() || c.groups.iter().any(|g| g.name == *name) + }); + (!known).then(|| format!("Unknown group '{name}' in '{category}'")) +} + +/// Refuse and stop unless the document has what the next effects name, asked at +/// apply time so a `:` line sees its earlier segments (`cli.md` §15). +#[derive(Debug)] +pub struct RequireKnown(pub Needs); +impl Effect for RequireKnown { + fn apply(&self, app: &mut App) { + if let Some(why) = unknown_name(app, &self.0) { + refuse_and_stop(app, why); + } + } + fn kind(&self) -> EffectKind { + EffectKind::ViewOnly + } +} + /// Skip the remaining effects in this `apply_effects` batch, for when a /// sequence's premise no longer holds. The flag resets per dispatch. #[derive(Debug)] diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index 473825f7..8853ff19 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -63,6 +63,10 @@ const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[ "refreshes the host's layout cache mid-batch; a client holds none (gotcha 23)", ), ), + ( + "RequireKnown", + Verdict::Off("a command-side name check that writes no state (gotcha 21, cli.md §15)"), + ), ( "AbortChain", Verdict::Off("control flow inside one apply_effects batch; it writes no state (gotcha 21)"), diff --git a/src/main.rs b/src/main.rs index 3bb31e30..5005eca9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -432,12 +432,16 @@ fn run_headless_commands(cmds: &[String], file: &Option) -> Result<()> for line in cmds { // The whole line is syntax-checked before any of it runs, exactly as // before; only CONSTRUCTION moved — `context/code/cli.md`. - if let Err(e) = command::parse_line(line) { - eprintln!("Parse error: {e}"); - exit_code = 1; - continue; - } - for segment in command::parse::segments(line) { + let segments = command::parse_line(line).and_then(|_| command::parse::segments(line)); + let segments = match segments { + Ok(segments) => segments, + Err(e) => { + eprintln!("Parse error: {e}"); + exit_code = 1; + continue; + } + }; + for segment in segments { let mut failed = None; let effects = { // Built against the cursor it is about to run against, so a diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index cf6f6771..bab79ffb 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -121,6 +121,58 @@ fn a_parse_error_still_exits_non_zero() { ); } +/// improvise-31oe.2 at the process boundary, with a control that exits 0. +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn an_unknown_category_axis_or_item_exits_non_zero_and_names_it() { + let scratch = Scratch::new("unknown_names"); + let doc = seeded(&scratch, "m.improv"); + let out = run_cmd(&["add-item Region East"], &doc); + assert!(out.status.success(), "the seed item: {}", stderr_of(&out)); + for (line, missing) in [ + ("set-axis Regoin row", "Regoin"), + ("set-page Region Nowhere", "Nowhere"), + ("set-axis Region sideways", "sideways"), + ] { + let out = run_cmd(&[line], &doc); + let said = stderr_of(&out); + assert!(!out.status.success(), "`{line}` exited 0; stderr={said:?}"); + assert!( + said.contains(missing), + "`{line}` did not name {missing}: {said:?}" + ); + } + let out = run_cmd(&["set-page Region East"], &doc); + assert!(out.status.success(), "the control: {}", stderr_of(&out)); +} + +/// improvise-31oe.3: one pipe-quoted argument, and a surplus one refused. +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_pipe_quoted_name_reaches_the_document_whole() { + let scratch = Scratch::new("pipes"); + let doc = seeded(&scratch, "m.improv"); + let out = run_cmd(&["create-view |By area|"], &doc); + assert!(out.status.success(), "{}", stderr_of(&out)); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!(text.contains("## View: By area\n"), "{text}"); + assert!( + !text.contains("|By"), + "a pipe reached the view name: {text}" + ); + + let out = run_cmd(&["create-view By area"], &doc); + let said = stderr_of(&out); + assert!( + !out.status.success(), + "a surplus argument exited 0: {said:?}" + ); + assert!( + said.contains("\"area\""), + "the refusal does not name it: {said:?}" + ); +} + /// Diagnostics go to stderr so that stdout stays free for a data output, and /// so `2>/dev/null` is the whole suppression story. `context/code/cli.md`. #[test]