diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index d72be6a9..dad4cb72 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -154,6 +154,59 @@ non-atomic rules produce the whitespace-separated tokens pest's implicit with three or more alternatives (nested `Choice`s) pick deeper branches less often — good enough for random generation, not a uniform distribution. +### The category kind is a line, not a header suffix (improvise-dy3j) + +`CategoryKind::Label` had no spelling in the format at all. `format_md` wrote +every category as a bare `## Category: Name` and `build_category` read every +section back through `add_category`, so the kind was dropped on the way out +**and** on the way in. A round-trip equality test cannot see a loss both sides +share, which is why the existing suite was silent — the same shape as the +`Formula::raw` traps above, and the reason the new tests assert the *parsed* +`Category::kind` rather than the emitted text. + +The consequence was a lost document, not a lost flag. `Label` sits outside +`MAX_CATEGORIES = 12` on purpose, and the import wizard's own precondition +scores a label as zero, so it will happily build twelve categories plus a +label. On load every label came back `Regular`, the thirteenth was refused, and +`parse_md` failed: **any document where regular + label exceeded twelve was +permanently unloadable.** Below the limit the failure was quieter — a label +silently became a pivot category, entering `regular_category_names()` pickers +and gaining axis auto-assignment. + +**The ticket proposed `## Category: Note [label]`, and the grammar forbids +it.** A section header takes its name as the **whole line**, totally +(improvise-pczv) — `header_name` is `pipe_quoted ~ &NEWLINE | line_name` — so a +suffix is not a suffix, it is part of a category literally named `Note +[label]`. The two readings are indistinguishable, and adopting the suffix would +have made a legal name unspellable while quietly re-opening the ambiguity +`header_name` exists to close. + +So the kind is an **entry inside the section**, beside the item lines: + +``` +## Category: Note +kind: label +- see the record view +``` + +Three properties earned it. It uses a **real token** (`"kind: "`), not a +lookahead, because the grammar-walking generator reads the grammar through +`pest_meta` and *skips predicates* — a rule guarded only by a lookahead is a +rule the generator can violate, which is why `WHERE` carries `sp+`. It cannot +collide with `item_list`, `grouped_item` or `group_hierarchy`, each of which +needs a `- ` or `> ` lead. And **absence is the compatible reading**: `kind_kw` +spells only `label`, the writer emits the line only for a label, so every file +written before this commit parses unchanged and every pivot category's section +is byte-identical to what it was. That is also why `parse_kind_kw` is an +exhaustive match returning `Result` rather than a `bool` read off the rule's +mere presence — a second keyword must fail loudly at the walker, not be read as +`label` because it arrived on a `kind_line`. + +One case the format already allowed and this had to answer: two +`## Category:` sections naming one category are one category (improvise-349m), +merged in pass 1. Absence is not a claim, so the merge keeps a stated kind +rather than letting a later kindless section erase it. + ### The tree walkers are infallible by construction Every `build_*` in `parser.rs` operates on a `Pair` pest has already diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 6043bff6..3b08e477 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1288,3 +1288,69 @@ the parse would fail the test for the wrong reason. The absent case, so the field above is something the file carried rather than a constant the walker invents. Without it, a walker hard-coding `Some(".4")` would pass the test above. + +### a_label_category_survives_the_round_trip_as_a_label + +`CategoryKind::Label` had no spelling in `.improv` at all: `format_md` wrote +every category as a bare `## Category: Name` and `build_category` read every +section back through `Workbook::add_category`. The kind was dropped on the way +out *and* on the way in, so the loss was symmetric and no round-trip equality +test could see it — the same shape the `Formula::format` entry above records. +Reproduced at `d46ec59`: a workbook with `Region` (regular) and `Note` (label) +came back with `Note.kind == Regular`. + +The test asserts the **parsed** `Category::kind`, never the emitted line, for +that reason. Its second assertion — that `Region` still reads back `Regular` — +is the control: a reader that made every category a `Label` satisfies the first +assertion and fails this one. + +Faults injected, both against the fixed tree: + +- **Writer drops the line** (`if false` around `w!(out, "kind: label")`). Fails + `left: Regular right: Label`, and takes + `a_document_at_the_category_limit_plus_a_label_reloads` with it. +- **Reader ignores the kind it parsed** (`build_category` calls `add_category` + unconditionally, discarding `pc.kind`). Same two failures. This is the + discriminating one: the file text is *correct* in this variant, so a test + comparing emitted output passes. + +### a_document_at_the_category_limit_plus_a_label_reloads + +The P1 half, and the reason improvise-dy3j was not merely a lost flag. `Label` +is outside `MAX_CATEGORIES = 12` by design — `Model::add_label_category` has no +limit check, and the import wizard's `proposed_category_count` scores a label +as zero — so a document with twelve categories and one label is legally +constructible and the wizard will build one from any CSV carrying a free-text +column. Every label came back `Regular` on load, the thirteenth category was +refused, and `parse_md` returned +`Err("Maximum of 12 categories reached")`: **the document was permanently +unloadable.** Reproduced at `d46ec59` with exactly that error. + +The second assertion (`regular_category_names().len() == 12`) is what keeps the +first honest. A "fix" that simply raised or removed the limit would let the +document load while putting a high-cardinality label into the pivot count, and +this assertion fails on it. + +### a_category_section_with_no_kind_line_loads_as_regular + +The absent case: every `.improv` file written before this commit carries no +`kind:` line, and must still load. Without it, a walker that defaulted +`PCategory::kind` to `Label` would pass both tests above. + +The input is hand-written text rather than a round trip, deliberately — a round +trip cannot exhibit an absent line, because the writer under test is the thing +that would have written it. + +### a_loaded_label_category_is_not_on_a_pivot_axis + +The end-to-end check that the two halves of improvise-dy3j/cj4u agree: a label +is off every pivot axis in every view after a save and a load, including a view +created after the label. + +Worth stating what it does and does not witness. It is **not** a second witness +for the kind: `format_md` writes each view's axes explicitly, so `Note: none` +carries the seating across the file whatever the kind does. What it witnesses +is the *seating* — with the `improvise-cj4u` fix reverted, `create_view` +pivots `Note` onto Column in the source workbook, the file faithfully records +`Note: column`, and this test fails. It was written while cj4u was still open +and failed then, which is how that was established rather than assumed. diff --git a/crates/improvise-io/src/persistence/format.rs b/crates/improvise-io/src/persistence/format.rs index 1997009c..46836900 100644 --- a/crates/improvise-io/src/persistence/format.rs +++ b/crates/improvise-io/src/persistence/format.rs @@ -181,6 +181,11 @@ pub fn format_md(workbook: &Workbook) -> String { continue; } w!(out, "\n## Category: {}", quote_header(&cat.name)); + // The kind is written only where it is not the default, so a pivot + // category's section is byte-identical to before (improvise-dy3j). + if cat.kind == CategoryKind::Label { + w!(out, "kind: label"); + } // Items are written in insertion order, because that is the order the // grid's axes consume (gotcha 18). A run of bare items shares a line. let mut bare: Vec = Vec::new(); diff --git a/crates/improvise-io/src/persistence/improv.pest b/crates/improvise-io/src/persistence/improv.pest index 094086a3..b4b60135 100644 --- a/crates/improvise-io/src/persistence/improv.pest +++ b/crates/improvise-io/src/persistence/improv.pest @@ -36,7 +36,12 @@ category_section = { category_entry* } -category_entry = _{ group_hierarchy | grouped_item | item_list } +category_entry = _{ kind_line | group_hierarchy | grouped_item | item_list } + +// The category's kind. Absent means `regular` — the compatible reading of +// every file written before this line existed. context/code/formula-and-io.md. +kind_line = { "kind: " ~ kind_kw ~ NEWLINE ~ blank_lines } +kind_kw = @{ "label" } // Comma-separated bare items (no group): `- Food, Gas, Total` item_list = { diff --git a/crates/improvise-io/src/persistence/parse.rs b/crates/improvise-io/src/persistence/parse.rs index 963ed3b0..cb1e4cf0 100644 --- a/crates/improvise-io/src/persistence/parse.rs +++ b/crates/improvise-io/src/persistence/parse.rs @@ -7,7 +7,7 @@ use pest::iterators::{Pair, Pairs}; use pest_derive::Parser; use crate::formula::{AggFunc, parse_formula}; -use crate::model::category::Group; +use crate::model::category::{CategoryKind, Group}; use crate::model::cell::{CellKey, CellValue}; use crate::view::Axis; use crate::workbook::Workbook; @@ -27,6 +27,9 @@ struct PCategory { /// is load-bearing, so pass 2 must replay this list as written. items: Vec<(String, Option)>, group_parents: Vec<(String, String)>, + /// What a `kind:` line said, `Regular` where none did. Absence is not a + /// claim, so merging two sections keeps the stated one — context/code/formula-and-io.md. + kind: CategoryKind, } /// One `## View:` section as pass 1 read it. @@ -60,16 +63,21 @@ impl ParsedFile { name, items, group_parents, + kind, } = pc; match self.categories.iter_mut().find(|e| e.name == name) { Some(existing) => { existing.items.extend(items); existing.group_parents.extend(group_parents); + if kind != CategoryKind::Regular { + existing.kind = kind; + } } None => self.categories.push(PCategory { name, items, group_parents, + kind, }), } } @@ -164,9 +172,14 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result { name: section_title(&mut inner, "category_section")?, items: Vec::new(), group_parents: Vec::new(), + kind: CategoryKind::Regular, }; for entry in inner { match entry.as_rule() { + Rule::kind_line => { + let kw = next(&mut entry.into_inner(), "kind_line")?; + pc.kind = parse_kind_kw(kw.as_str())?; + } Rule::item_list => { for name_pair in entry.into_inner() { pc.items.push((extract_name(name_pair)?, None)); @@ -185,6 +198,15 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result { Ok(pc) } +/// The on-disk keyword for a category kind, exhaustively — the reader's half +/// of the `kind: label` line `format.rs` writes. +fn parse_kind_kw(kind: &str) -> Result { + match kind { + "label" => Ok(CategoryKind::Label), + _ => bail!("Unknown category kind: {kind}"), + } +} + /// The on-disk keyword for an aggregation, exhaustively — the reader's half of /// `format.rs::agg_keyword`. fn parse_agg_kind(kind: &str) -> Result { @@ -361,7 +383,10 @@ fn walk_file(file: Pair<'_, Rule>) -> Result { /// Register one category's items and groups, in file order. fn build_category(wb: &mut Workbook, pc: &PCategory) -> Result<()> { - wb.add_category(&pc.name)?; + match pc.kind { + CategoryKind::Label => wb.add_label_category(&pc.name)?, + _ => wb.add_category(&pc.name)?, + }; let cat = wb .model .category_mut(&pc.name) @@ -490,6 +515,7 @@ mod grammar_coverage { Rule::section => "silent: hoisted into file", Rule::category_section => "walk_category_section", Rule::item_list | Rule::grouped_item | Rule::group_hierarchy => "walk_category_section", + Rule::kind_line | Rule::kind_kw => "walk_category_section", Rule::category_entry => "silent: hoisted into category_section", Rule::formulas_section | Rule::formula_line | Rule::formula_raw => { "walk_formulas_section" diff --git a/crates/improvise-io/src/persistence/roundtrip_tests.rs b/crates/improvise-io/src/persistence/roundtrip_tests.rs index f0a05ad6..2e341495 100644 --- a/crates/improvise-io/src/persistence/roundtrip_tests.rs +++ b/crates/improvise-io/src/persistence/roundtrip_tests.rs @@ -4,7 +4,7 @@ use super::test_helpers::{coord, two_cat_model}; use super::{format_md, parse_md}; use crate::formula::parse_formula; -use crate::model::category::Group; +use crate::model::category::{CategoryKind, Group}; use crate::model::cell::CellValue; use crate::view::Axis; use crate::workbook::Workbook; @@ -1158,3 +1158,79 @@ fn parse_md_reads_a_formula_with_no_format_as_none() { let loaded = parse_md(&format_md(&m)).unwrap(); assert_eq!(loaded.model.formulas()[0].format, None); } + +// ── Category kinds (improvise-dy3j) ────────────────────────────────────── + +/// The kind is a parsed field, not a spelling the file happens to carry. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_label_category_survives_the_round_trip_as_a_label() { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_label_category("Note").unwrap(); + + let loaded = parse_md(&format_md(&wb)).unwrap(); + assert_eq!( + loaded.model.category("Note").unwrap().kind, + CategoryKind::Label + ); + // The control that keeps the assertion above from passing vacuously: a + // reader making every category a Label would clear it and fail here. + assert_eq!( + loaded.model.category("Region").unwrap().kind, + CategoryKind::Regular + ); +} + +/// A label sits outside `MAX_CATEGORIES`, so a document at the limit plus one +/// label must reload. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_document_at_the_category_limit_plus_a_label_reloads() { + let mut wb = Workbook::new("T"); + for i in 0..12 { + wb.add_category(format!("C{i}")).unwrap(); + } + wb.add_label_category("Note").unwrap(); + + let loaded = parse_md(&format_md(&wb)).expect("a label must not count against the limit"); + assert_eq!( + loaded.model.category("Note").unwrap().kind, + CategoryKind::Label + ); + assert_eq!(loaded.model.regular_category_names().len(), 12); +} + +/// A file written before the `kind:` line existed reads its categories as +/// `Regular` — the compatible reading, and what the writer still omits. +#[test] +fn a_category_section_with_no_kind_line_loads_as_regular() { + let text = "v2025-04-09\n\ + # Budget\n\ + \n\ + ## Category: Region\n\ + - North, South\n"; + let loaded = parse_md(text).unwrap(); + assert_eq!( + loaded.model.category("Region").unwrap().kind, + CategoryKind::Regular + ); +} + +/// The loader seats a label off every pivot axis, which is the view half of +/// the same fact (improvise-cj4u). +#[test] +fn a_loaded_label_category_is_not_on_a_pivot_axis() { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_label_category("Note").unwrap(); + wb.create_view("Other"); + + let loaded = parse_md(&format_md(&wb)).unwrap(); + for (name, view) in &loaded.views { + assert_eq!( + view.axis_of("Note"), + Axis::None, + "view {name} pivoted a label category" + ); + } +}