Merge gh/main into fix/31oe-cmdlang; renumber cli.md §13/§14 to §14/§15
#321 added its own cli.md section 13, so this PR's sections become 14 (pipe quoting, surplus arguments) and 15 (names the document lacks), with every citation renumbered. The stale descriptions of the deleted unknown_* helpers in cli.md, command.md and the test evidence now describe RequireKnown, and section 15 says a refused name stops the rest of a `:` line but not a headless one. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
+51
-10
@@ -393,7 +393,40 @@ The headless path matters most, because the code-maat pipeline
|
||||
pins the views, the sparse Data lines and the byte-identical second run, with
|
||||
the first run's output sitting in the folder it reads.
|
||||
|
||||
## 13 — the command language reads `|pipe quoting|`, and a surplus argument is refused (improvise-31oe.3)
|
||||
## 13 — a headless import refuses rather than saving less than was asked (improvise-8jk6.4, 8jk6.5, 8jk6.6)
|
||||
|
||||
The user treats the headless command and model layer as a product of its own,
|
||||
and the scripted `import --no-wizard` is where it meets a pipeline. The
|
||||
repo-stats pipeline (improvise-1xfb.5.1) found three ways that import saved
|
||||
something other than what the flags asked for, and still exited 0. The
|
||||
reasoning for each is in `context/code/formula-and-io.md`, "A CSV field is
|
||||
text, and the field's role reads it" and "An import refuses what it cannot
|
||||
keep". The CLI's contract:
|
||||
|
||||
- **`--category` keeps the CSV text.** A forced category's items are the
|
||||
file's text, verbatim: `00565796` stays `00565796`, and `220944e2` stays
|
||||
`220944e2`. Measures still read as numbers. A JSON number has no source
|
||||
text, so it is named as JSON writes it (`1e3` is `1000.0`).
|
||||
- **An unparseable `--formula` is a refusal.** It exits non-zero and writes
|
||||
nothing. Every bad formula is named on stderr, each with its parse error.
|
||||
This is §10's rule: the request was not met, so the exit code says so. The
|
||||
wizard refuses the same text at the formula step, with the same wording.
|
||||
- **An import that kept no record is a refusal.** It exits non-zero, writes
|
||||
nothing, and says why: every record was skipped as not a JSON object, or
|
||||
the source had none.
|
||||
- **An import that skipped *some* records exits 0 by default.** It writes the
|
||||
model and warns with a count. With `--strict` (the user's ruling on PR #321)
|
||||
any skipped record is a refusal instead: exit 1, the same count named, and
|
||||
nothing written. The argument is in `formula-and-io.md`.
|
||||
|
||||
Two refusals bail in `run_headless_import` before the build: the `--strict`
|
||||
skip check (`src/main.rs:305`) and the `--on-duplicate refuse` collision
|
||||
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.
|
||||
@@ -454,7 +487,7 @@ Total = Widget + Gadget` still says.
|
||||
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.
|
||||
|
||||
## 14 — a command that names something the document lacks is refused (improvise-31oe.2)
|
||||
## 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
|
||||
@@ -464,14 +497,15 @@ request was not met.
|
||||
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**, through
|
||||
`effect::refuse` in `execute`, because only the context knows what the
|
||||
document holds. `unknown_category` asks the active view (`try_axis_of`). That
|
||||
is exactly the condition under which `View::set_axis` was a silent no-op.
|
||||
`unknown_item` asks `effective_item_names`, so a formula target is a known
|
||||
`_Measure` item. `unknown_group` asks both an item's own group and the
|
||||
category's `groups`. `add_item_in_group` registers no `Group`, and a parent
|
||||
group has no items, so either one alone misses a real group.
|
||||
- **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
|
||||
@@ -493,6 +527,13 @@ stay non-refusing, since a refusing effect there becomes a frame rejection
|
||||
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
|
||||
|
||||
@@ -2999,7 +2999,7 @@ and the re-export keeps `crate::command::key::…` paths working.
|
||||
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` §13).
|
||||
and the quoting cannot disagree (improvise-31oe.3, `context/code/cli.md` §14).
|
||||
|
||||
### `cmd/core.rs`
|
||||
|
||||
@@ -3020,7 +3020,7 @@ 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` §13).
|
||||
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
|
||||
@@ -3380,13 +3380,13 @@ 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 or group checks it in `execute` through
|
||||
`unknown_category`, `unknown_item` or `unknown_group`, `switch-view` checks
|
||||
`workbook.views`, and each emits
|
||||
`effect::refuse` in place of its effect when the document lacks it
|
||||
(improvise-31oe.2). The effects stay silent about unknown names, because they
|
||||
are also the wire's mutation path. Why the check sits here and not there:
|
||||
`context/code/cli.md` §14.
|
||||
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)
|
||||
|
||||
|
||||
@@ -658,6 +658,118 @@ one proves nothing.
|
||||
backend rather than only a file (improvise-fuj); the path-taking wrappers remain
|
||||
for the native CLI.
|
||||
|
||||
### A CSV field is text, and the field's role reads it (improvise-8jk6.4)
|
||||
|
||||
The parser used to type every field as it read it: text that parsed as an
|
||||
`i64` or a finite `f64` became a `Value::Number`. That is a lossy step, and it
|
||||
ran before anyone knew what the column was for. A column the user then forced
|
||||
to Category (`--category`, or the wizard's `c`) took its item names from the
|
||||
number's display, not from the file. So commit `00565796` became item `565796`,
|
||||
and `220944e2` was read as scientific notation and became `22094400.0`. The
|
||||
repo-stats pipeline (improvise-1xfb.5.1) found it.
|
||||
|
||||
CSV has no types. Whether `00565796` is a number depends on whether its column
|
||||
is a measure, and the parser cannot know that. So the parser now yields every
|
||||
non-empty field as a `Value::String` of its text (blank is still `Null`), and
|
||||
two readers apply the old numeric rule, `csv_parser::number_in`, only where a
|
||||
number is wanted:
|
||||
|
||||
- **Analysis** proposes a kind from a typed copy of the records
|
||||
(`Scalars::typed_record`), so a numeric CSV column is still proposed as a
|
||||
Measure, exactly as before.
|
||||
- **A measure** takes its value through `Scalars::number`.
|
||||
|
||||
A category or a label is named from the text itself, so it is verbatim.
|
||||
|
||||
`ImportPipeline` carries the reading as a private `scalars: Scalars`:
|
||||
`Typed` for JSON and `Text` for CSV. JSON cannot share the text rule, because
|
||||
a JSON string is text by the file's own say. `"02139"` in JSON is a zip code,
|
||||
and reading it as a number would propose it as a Measure. A JSON number
|
||||
forced to Category has no source text to keep, because serde_json does not
|
||||
keep one. It is named by `Value`'s own display, which is what writing it back
|
||||
as JSON would say: `42`, `1.5`, and `1000.0` for `1e3`.
|
||||
|
||||
**The parser returns `CsvRecords`, not `Vec<Value>`.** It derefs to
|
||||
`[Value]`, so a caller can read it. It converts to a `Vec` only inside the
|
||||
crate, and the only public way into a pipeline is `ImportPipeline::from_csv`.
|
||||
This is not caution for its own sake. When the parser first changed, eleven
|
||||
tests in four files failed. Every one handed parser output to
|
||||
`ImportPipeline::new`, the JSON constructor, which would read every column as
|
||||
text and propose `Revenue` as a Category. Production had the same shape at all
|
||||
three CSV entry points: `main.rs`'s single file, `ImportPipeline::from_merge`
|
||||
(the merge path, from PR #314), and `ImportJsonHeadless`. `from_merge` merged
|
||||
in with `Self::new` and now calls `Self::from_csv`. Now that mistake does not
|
||||
compile. Two holes remain, and
|
||||
both take a deliberate step. `ImportPipeline::records` is still a public
|
||||
`Vec<Value>`, and a test helper once copied it out of a CSV pipeline into a
|
||||
JSON one. And `CsvRecords` derefs to a slice that `.to_vec()` will copy.
|
||||
|
||||
### An import refuses what it cannot keep (improvise-8jk6.5, improvise-8jk6.6)
|
||||
|
||||
Two failures used to end in an import that saved and reported success.
|
||||
|
||||
**A formula that does not parse.** `build_model_with_warnings` ran
|
||||
`if let Ok(formula) = parse_formula(..)`, so a bad `--formula` vanished. The
|
||||
headless run then saved and exited 0, and the wizard kept the bad text in its
|
||||
list and dropped it the same way at build. Both now go through one
|
||||
function, `import_formula`, whose error names the formula and carries pest's
|
||||
parse error: `Formula error in 'Y = x +': …`. The prefix matches the TUI
|
||||
formula minibuffer's `Formula error:`.
|
||||
|
||||
- `ImportPipeline::parsed_formulas` parses them all. If any fails, it refuses
|
||||
and names **every** formula that failed, one per line (a message the user
|
||||
needs is never truncated). The build calls it before creating anything, so a
|
||||
refusal writes nothing.
|
||||
- The wizard checks at `confirm_formula`, the one place the user can still fix
|
||||
the text (gotcha 22). A formula that does not parse stays in the buffer with
|
||||
the error as the step's message, and never enters the list.
|
||||
|
||||
**Nothing kept.** The one skip left after improvise-8jk6.2 is a record that
|
||||
is not a JSON object. When every record is skipped, there are no fields, so
|
||||
there are no proposals and no categories. The build did refuse that case, but
|
||||
by accident, with "At least one category must be accepted". That tells the
|
||||
user to change a field's kind when there are no fields. The ticket's own repro
|
||||
(`[1, 2, "three"]`) already exited 1 on the lpzu base, so the headline did not
|
||||
reproduce there. What did reproduce was the reason. `nothing_to_import` now
|
||||
states the rule and runs before the category check: `Nothing to import: all 3
|
||||
records were skipped as not JSON objects`, or `the source has no records`. The
|
||||
wizard asks it first on `ReviewProposals`, where another array path can still
|
||||
be chosen.
|
||||
|
||||
One case meets PR #314's "a header-only file is still a file". A folder whose
|
||||
CSVs are **all** header-only has no records, only sources. Built from
|
||||
`from_merge`'s seeded proposals alone, it would save `File` items, empty
|
||||
columns and one empty records view per file. It is refused here, because it
|
||||
kept no record. A header-only file *beside* files with rows is unaffected,
|
||||
since some record was kept. The user confirmed this on PR #321: "Refuse (as
|
||||
#321 does)".
|
||||
|
||||
**A partial skip stays a warning.** Some records kept and some skipped still
|
||||
builds, exits 0, and warns with a count: `Skipped 1 of 3 records that were not
|
||||
JSON objects`. The argument: a non-object in an array of records is not a
|
||||
record that failed to import. It is structurally not a record, such as a
|
||||
trailing `null` or a stray scalar in a hand-edited file. Every record that
|
||||
*is* a record was kept, and the count is on stderr for a script to read.
|
||||
Making it fatal would fail every import that carries one stray element.
|
||||
Making it a distinct non-zero exit that still writes gives a contract where
|
||||
`import … && next` stops even though the file was written.
|
||||
|
||||
The user ruled on PR #321: "Add a --strict flag". The default stays exit 0
|
||||
with the counted warning, pinned by
|
||||
`an_import_that_skips_some_records_warns_with_a_count_and_still_writes`.
|
||||
`import --no-wizard --strict` turns any skipped record into a refusal:
|
||||
`--strict: Skipped 1 of 3 records that were not JSON objects`, exit 1.
|
||||
|
||||
**A strict refusal writes nothing.** A refusal that still wrote the file would
|
||||
be the half-met contract argued against above: `&&` stops while the output
|
||||
sits there, and a script that checks only the file gets a model that `--strict`
|
||||
existed to prevent. So the check runs in `run_headless_import` before the
|
||||
build. It reads `ImportPipeline::skip_warning`, the same function the build's
|
||||
warning comes from, so the two cannot disagree about what was skipped.
|
||||
"Skipped" here means a record that is not an object, the only skip left after
|
||||
8jk6.2. An unparsed date or a short CSV row is a warning about a record that
|
||||
was kept, and `--strict` does not refuse it.
|
||||
|
||||
### A folder of CSVs is one sparse model, with a view per file (improvise-lpzu)
|
||||
|
||||
`improvise import <folder>` exists for improvise-1xfb.5.1. code-maat writes one
|
||||
|
||||
Reference in New Issue
Block a user