diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 81679ac3..f524a876 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -501,6 +501,41 @@ test flips the category back to `Page` afterwards, so it distinguishes "omits The fixture puts a category on each of `Page`, `Filter`, `Row` and `None` so the result separates the two questions rather than merely counting. +### on_category_removed_forgets_a_collapsed_group + +improvise-qx1z arm 2. `View::on_category_removed` cleared `category_axes`, +`page_selections` and `hidden_items` — but not `collapsed_groups`. The other +three lines being right there is what makes this an omission rather than a +decision: collapse `Month/Q1`, delete the category, re-add it with `Jan[Q1]`, +and the group comes back **pre-collapsed** with nothing saying why. Reachable +from shipped keys, through the category panel. + +Fault injected: delete the added line; the test fails at +`is_group_collapsed("Month", "Q1")`. The test re-adds the category after +removing it, because a view that merely *forgot the axis* would also report +`false` for a category it no longer knows — asking the question after the +re-add is what separates "cleared" from "not currently registered". + +### on_item_removed_forgets_a_hidden_item + +improvise-qx1z arm 1, at the `View` level. `Model::remove_item` removes the item +and its cells and **touches no view at all**, so `hidden_items["Region"]` kept +`"East"`: re-adding the item put it in the model and in the category panel while +leaving it absent from the grid, with no indication why. + +The fixture hides **two** items and removes one. Without the second, a fix that +dropped the whole category entry — `hidden_items.remove(cat_name)` — would pass +while destroying unrelated state; the surviving `West` assertion is the input +that separates "forget this item" from "forget this category". + +### on_item_removed_forgets_a_page_selection_naming_it + +The third question improvise-qx1z asked to check while there, and the answer was +the same: a page selection naming a removed item survived it. Asserted in both +directions in one test — removing a *different* item must leave the selection +alone — because a hook that cleared the selection unconditionally would satisfy +the removal half and silently reset the user's page on every unrelated delete. + ## `view/layout.rs` ### prune_axis_items_indexes_data_positions_not_visual_ones diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 4e77123e..ab7581cd 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -510,6 +510,170 @@ honest — without it, "the writer quotes it" would be consistent with the reade also accepting the unquoted form. The trailing precondition (the same line quoted *does* parse) stops it passing because of some unrelated syntax error. +### prune_empty_survives_the_roundtrip + +improvise-qu4a. `View::prune_empty` was never written to disk: `format.rs` wrote +`format:`, `hidden:` and `collapsed:` for a view and nothing else, and +`improv.pest`'s `view_entry` had no `prune` alternative. Pressing `P`, saving +and reopening brought every pruned row and column back — and the toggle marks +the document dirty, so the user is invited to save the setting that is about to +be discarded. + +The reason nothing caught it is the shape worth remembering: it is **invisible +to `fmt(parse(x)) == fmt(parse(fmt(parse(x))))`, because both sides drop it** +(`doctrine(a_roundtrip_equality_is_blind_to_a_loss_both_sides_share)`). So this +test asserts the **flag**, never the output's stability. Fault injected: delete +the two-line `w!` in `format_md`; the test fails at the `prune: true` assertion, +and the two controls below still pass — which is exactly why they are controls +and not the evidence. + +#### a_view_that_does_not_prune_writes_no_prune_line + +The control, stated separately because it is the direction the default supplies. +A writer that emitted `prune: true` unconditionally passes the test above and +fails this one. Not evidence of the bug; evidence the fix is not one-directional. + +#### a_file_without_a_prune_line_does_not_prune + +The compatibility reading, decided rather than defaulted: a file written before +the line existed means `false`, which is what happens today. `improv.pest` +spells `prune: true|false` rather than making the line's presence the boolean, +so the grammar-walking generator can emit `prune: false` and it round-trips to +`false` through a writer that omits it. + +### a_file_naming_its_own_views_gains_no_default + +improvise-m4rv. `Workbook::new` seeds a `Default` view and `on_category_added` +assigns every parsed category into **every** existing view, so a file naming +only `Plan` parsed into *two* views — the second auto-laid-out and never in the +file. Two consequences, the second worse: the shipped demo's view panel showed a +phantom row **at index 0**, so the first `V`+`Enter` a visitor pressed took them +off the curated view; and open-and-save with no edits rewrote the user's +document, appending the whole `## View: Default` block permanently. + +`examples/demo.improv` was unaffected only because its view *is* named +`Default`, which is why the bug survived the shipped-document tests. + +Fault injected: remove the `drop_unnamed_default` call in `build_workbook`; the +test fails with `["Default", "Plan"]`. The fix is narrow on purpose — it drops +`Default` only when the file named **at least one** view and none of them was +`Default`, so the two cases in the control below are untouched. + +#### a_file_that_names_default_itself_keeps_it_alongside_its_siblings + +The control the fix must not break, and **the second view is the whole point**. +This entry replaces a vacuous one, found by review injecting +`default_is_a_phantom = true` — the over-broad "always drop Default" — and +watching the control **pass**. It passed because `Workbook::delete_view` refuses +the last view: with `Default` alone in the map the drop was refused by a guard +one level away from the condition under test, so the test answered *does +`delete_view` refuse the last view* where it meant *does `drop_unnamed_default` +decline to fire*. Naming `Default` **and** `Other` gives the map two entries, so +the wrong condition really does delete and the test really can fail. (Two +pre-existing tests, `parse_md_round_trips_multiple_views` and +`parse_md_order_independent_new_view_before_categories`, also catch it — the +repo was covered, but not by the test that claimed to be the control.) + +#### a_file_naming_no_view_keeps_the_seeded_default + +The legacy reading, kept separate and **deliberately weaker**: a file with no +`## View` section at all still opens on the seeded view. It cannot be made +discriminating the way its neighbour was — with no view named there is only ever +one in the map, so `delete_view`'s last-view refusal masks any condition. It is +a statement of behaviour, not evidence of the fix. + +#### a_category_named_prune_is_not_read_as_the_prune_flag + +`prune: true` and `prune: row` differ only in the value, and `view_entry` tries +`prune_line` **before** `axis_line` — so a category literally named `prune` +round-trips only because `bool_kw` is `"true" | "false"` and fails on an axis +keyword. Safety rests on the disjointness of two keyword sets, which nothing +else checks, which is why the test exists. + +**Its first version was vacuous, and the injection is what showed it.** Feeding +the rejected presence-style design — `prune_line = { "prune: " ~ rest_of_line }`, +which swallows `prune: row` whole — left the suite green, because the fixture put +the category on `Row` and `Workbook::add_category` → `View::on_category_added` +**re-derives** `Row` for the first regular category on reload. The axis matched +by reinvention rather than by being read. The fixture now uses `Page` plus a page +selection, neither of which any default supplies, and the same injection fails +it. + +### the_aggregation_section_is_written_in_a_stable_order + +improvise-ppru's companion. `Model::measure_agg` is a `HashMap`, and an index +whose iteration order is hash order is not an order (gotcha 18) — writing it +unsorted would give a different `.improv` file per process, the same class of +defect as `analyze_records`. The test pins the three lines in name order and +then asserts `format(parse(text)) == text`. Fault injected: drop the +`sort_by_key`; the assertion fails on most runs, which is itself the argument +for pinning it rather than trusting a single green run. + +### a_removed_measures_aggregation_is_not_written + +Found by **review of this branch**, not by a ticket, and it is improvise-qx1z's +own defect one layer down: `Model::remove_item` removed the item and its cells +and left `measure_agg` holding an entry for a measure that no longer exists. + +It was harmless while improvise-ppru was unfixed, because nothing persisted the +map — the stale entry evaporated on reload. **Persisting it made it durable**, so +the document grew a `## Aggregation` line naming a measure it does not have, and +re-adding that measure brought back an aggregation nobody asked for. A fix that +introduces a format for state that outlives what it names is how a latent bug +becomes a permanent one; the sweep had to ship with the writer. + +The fixture carries **two** measures and removes one, so the assertion separates +"forget this measure" from "clear the map" — the same shape +`on_item_removed_forgets_a_hidden_item` needs for the view half. Fault injected: +delete the `measure_agg.remove` line; the test fails on the written text before +it ever reaches the reader. + +### an_interleaved_category_keeps_its_item_order + +improvise-3yv3. `format_md` collected every ungrouped item into `bare` and +emitted that whole list *before* any grouped item, so a category built as +`[grouped, ungrouped]` came back `[ungrouped, grouped]`. Grid axis ordering +consumes item insertion order (gotcha 18), so reopening a document silently +rearranged its rows and columns. + +The fixture interleaves deliberately — `a, b, g1[G], c, g2[G]` — because the +ticket's own two-item reproduction cannot tell "writes grouped items last" from +"writes each run in place": with one item of each kind, *both* orders are a +single bare run followed by a single grouped one. The test asserts the four +emitted lines, not just the parsed order, so a writer that happened to re-sort +correctly by accident still fails. Fault injected: restore the two-vector split; +the line assertion fails with `["- a, b, c", "- g1[G]", "- g2[G]"]`. + +### a_cell_with_no_coordinates_round_trips + +improvise-pnf5, the P1. A cell keyed by the empty `CellKey` is a legal model +state — it is what `AddRecordRow` leaves while a records row is unfinished, and +it is reachable from the shipped `r` then `o` keys. `format_md` wrote it as +`" = 0"` and `coord_list` required at least one coordinate, so **saving in +records mode overwrote a good document with one the application could not +reopen**. `CleanEmptyRecords` runs on leaving records mode, not before `Save`. + +The fix is a spelling rather than a discard, because the ticket is explicit that +legitimate data must not be dropped: `data_coords = _{ (coord_list ~ " ")? }` +gives the scalar cell the line `= 42`. It is unambiguous under PEG because +`coord` needs a `name`, and `bare_name` cannot start with `=`, so the optional +group fails cleanly and the `"= "` literal matches. + +Fault injected: revert the writer's `match` to the old `"{} = {}"`; the test +fails at the `\n= 42\n` assertion, and `parse_md` then rejects its own writer's +output. The trailing `format_md(&back) == text` is what makes this a round trip +rather than a parse check. + +#### slash_in_hidden_name_survives_roundtrip + +**Not a bug.** improvise-nvq4 (external report) claimed a `/` in an item or +group name corrupts the `hidden:`/`collapsed:` line, whose format is +`Cat/Item`. It does not reproduce: `hidden_line` reads two `name`s, a `name` is +`pipe_quoted | bare_name`, and `bare_name` excludes `/` — so `quote_name` +already writes `hidden: C/|A/B|` and `extract_name_pair` reads it back. The test +is kept as a **passing control** over both maps, so the claim cannot be re-filed +without someone first watching this pass. + --- ## `improvise-io/src/persistence/export.rs` diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 49d767ae..badcab92 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -351,6 +351,11 @@ impl Model { if let Some(cat) = self.categories.get_mut(cat_name) { cat.remove_item(item_name); } + // The measure's aggregation is part of what the measure IS, so it goes + // with it — improvise-qx1z's asymmetry, on the model side. + if CategoryKind::of_name(cat_name).is_measure() { + self.measure_agg.remove(item_name); + } let to_remove: Vec = self .data .iter_cells() diff --git a/crates/improvise-core/src/view/mod.rs b/crates/improvise-core/src/view/mod.rs index 5e061ce2..b6f45941 100644 --- a/crates/improvise-core/src/view/mod.rs +++ b/crates/improvise-core/src/view/mod.rs @@ -7,4 +7,4 @@ pub use layout::{ AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow, synthetic_record_info, }; -pub use types::{DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View}; +pub use types::{DEFAULT_VIEW, DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View}; diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index eace86e6..1efeeb3e 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -13,6 +13,10 @@ fn default_prune() -> bool { /// they put the never-persisted `_Index`/`_Dim` on axes (improvise-60z). pub const VIRTUAL_VIEW_NAMES: [&str; 2] = ["_Records", "_Drill"]; +/// The view `Workbook::new` seeds. `parse_md` must name the same one to tell a +/// seeded view from one the file asked for (improvise-m4rv). +pub const DEFAULT_VIEW: &str = "Default"; + /// Name of the view records mode switches to. pub const RECORDS_VIEW: &str = VIRTUAL_VIEW_NAMES[0]; /// Name of the view drill-down switches to. @@ -94,6 +98,22 @@ impl View { self.category_axes.shift_remove(cat_name); self.page_selections.remove(cat_name); self.hidden_items.remove(cat_name); + self.collapsed_groups.remove(cat_name); + } + + /// Forget an item this view names. A hidden or paged-to item that outlives + /// its removal comes back hidden when re-added (improvise-qx1z). + pub fn on_item_removed(&mut self, cat_name: &str, item_name: &str) { + if let Some(items) = self.hidden_items.get_mut(cat_name) { + items.remove(item_name); + } + if self + .page_selections + .get(cat_name) + .is_some_and(|s| s == item_name) + { + self.page_selections.remove(cat_name); + } } pub fn set_axis(&mut self, cat_name: &str, axis: Axis) { @@ -475,6 +495,54 @@ mod tests { assert!(!v.is_group_collapsed("Cat1", "G2")); } + /// Arm 2 of improvise-qx1z: the other three lines of `on_category_removed` + /// were right there. Evidence: `context/test-evidence/core.md`. + #[test] + fn on_category_removed_forgets_a_collapsed_group() { + let mut v = View::new("V"); + v.on_category_added("Month"); + v.toggle_group_collapse("Month", "Q1"); + v.on_category_removed("Month"); + v.on_category_added("Month"); + assert!( + !v.is_group_collapsed("Month", "Q1"), + "a re-added category came back with its group collapsed" + ); + } + + /// Arm 1 of improvise-qx1z, at the `View` level: removing an item must not + /// leave it hidden, or re-adding it brings back an invisible row. + #[test] + fn on_item_removed_forgets_a_hidden_item() { + let mut v = View::new("V"); + v.on_category_added("Region"); + v.hide_item("Region", "East"); + v.hide_item("Region", "West"); + v.on_item_removed("Region", "East"); + assert!(!v.is_hidden("Region", "East"), "East is still hidden"); + assert!( + v.is_hidden("Region", "West"), + "on_item_removed must forget one item, not the category" + ); + } + + /// The third question improvise-qx1z asked to check while there: a page + /// selection naming a removed item does not survive it either. + #[test] + fn on_item_removed_forgets_a_page_selection_naming_it() { + let mut v = View::new("V"); + v.on_category_added("Region"); + v.set_page_selection("Region", "East"); + v.on_item_removed("Region", "West"); + assert_eq!( + v.page_selection("Region"), + Some("East"), + "removing a different item must leave the selection alone" + ); + v.on_item_removed("Region", "East"); + assert_eq!(v.page_selection("Region"), None); + } + #[test] fn hide_and_show_item() { let mut v = View::new("Test"); diff --git a/crates/improvise-core/src/workbook.rs b/crates/improvise-core/src/workbook.rs index b47e120d..243708de 100644 --- a/crates/improvise-core/src/workbook.rs +++ b/crates/improvise-core/src/workbook.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; use crate::model::Model; use crate::model::category::{CategoryId, ItemId}; -use crate::view::{Axis, View}; +use crate::view::{Axis, DEFAULT_VIEW, View}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Workbook { @@ -22,11 +22,11 @@ impl Workbook { pub fn new(name: impl Into) -> Self { let model = Model::new(name); let mut views = IndexMap::new(); - views.insert("Default".to_string(), View::new("Default")); + views.insert(DEFAULT_VIEW.to_string(), View::new(DEFAULT_VIEW)); let mut wb = Self { model, views, - active_view: "Default".to_string(), + active_view: DEFAULT_VIEW.to_string(), }; for view in wb.views.values_mut() { for cat_name in wb.model.categories.keys() { @@ -91,6 +91,9 @@ impl Workbook { /// ignored, matching [`Model::remove_item`]. pub fn remove_item(&mut self, cat_name: &str, item_name: &str) { self.model.remove_item(cat_name, item_name); + for view in self.views.values_mut() { + view.on_item_removed(cat_name, item_name); + } } fn category_mut(&mut self, cat_name: &str) -> Result<&mut crate::model::category::Category> { @@ -260,6 +263,27 @@ mod tests { ); } + /// Arm 1 of improvise-qx1z through the facade: `Model::remove_item` touches + /// no view, so the cross-view sweep is `Workbook`'s job (gotcha 6). + #[test] + fn remove_item_unhides_it_in_every_view() { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.create_view("Other"); + for name in ["Default", "Other"] { + wb.views.get_mut(name).unwrap().hide_item("Region", "East"); + } + wb.remove_item("Region", "East"); + wb.add_item("Region", "East").unwrap(); + for name in ["Default", "Other"] { + assert!( + !wb.views[name].is_hidden("Region", "East"), + "{name} brought East back hidden" + ); + } + } + #[test] fn remove_item_on_unknown_category_is_a_no_op() { let mut wb = Workbook::new("Test"); diff --git a/crates/improvise-io/src/persistence/format.rs b/crates/improvise-io/src/persistence/format.rs index 0ccd3dd3..1997009c 100644 --- a/crates/improvise-io/src/persistence/format.rs +++ b/crates/improvise-io/src/persistence/format.rs @@ -7,6 +7,7 @@ use crate::workbook::Workbook; // The one home of the pipe-quoting rule (improvise-i2m5); a name written here // must also survive `parse_formula`. See `context/code/formula-and-io.md`. +use crate::formula::AggFunc; use crate::formula::quote::{pipe_quote, quote_name}; /// A name as a section header spells it: the whole line, so no pipes where a @@ -22,6 +23,18 @@ fn quote_header(name: &str) -> String { } } +/// The on-disk keyword for an aggregation, exhaustively — a new `AggFunc` +/// variant fails the build here rather than being written as something else. +fn agg_keyword(agg: &AggFunc) -> &'static str { + match agg { + AggFunc::Sum => "sum", + AggFunc::Avg => "avg", + AggFunc::Min => "min", + AggFunc::Max => "max", + AggFunc::Count => "count", + } +} + fn format_number(n: f64) -> String { if n.is_infinite() { return if n.is_sign_positive() { @@ -108,6 +121,9 @@ pub fn format_md(workbook: &Workbook) -> String { if !view.number_format.is_empty() { w!(out, "format: {}", view.number_format); } + if view.prune_empty { + w!(out, "prune: true"); + } for (prefix, map) in [ ("hidden", &view.hidden_items), ("collapsed", &view.collapsed_groups), @@ -137,6 +153,19 @@ pub fn format_md(workbook: &Workbook) -> String { } } + // ── Aggregation ────────────────────────────────────────────────── + + // Sorted: `measure_agg` is a `HashMap` and hash order is not an order + // (gotcha 18) — two saves of one document must agree. + let mut aggs: Vec<_> = model.measure_agg.iter().collect(); + aggs.sort_by_key(|(name, _)| name.as_str()); + if !aggs.is_empty() { + w!(out, "\n## Aggregation"); + for (name, agg) in aggs { + w!(out, "- {}: {}", quote_name(name), agg_keyword(agg)); + } + } + // ── Categories (items comma-separated on one line) ─────────────── // Collect formula targets so we can exclude them from _Measure items let formula_targets: std::collections::HashSet<&str> = model @@ -152,8 +181,9 @@ pub fn format_md(workbook: &Workbook) -> String { continue; } w!(out, "\n## Category: {}", quote_header(&cat.name)); + // 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(); - let mut grouped: Vec = Vec::new(); for item in cat.items.values() { // For _Measure, skip items that are formula targets // (they'll be recreated from the ## Formulas section) @@ -163,16 +193,19 @@ pub fn format_md(workbook: &Workbook) -> String { continue; } match &item.group { - Some(g) => grouped.push(format!("{}[{}]", quote_name(&item.name), quote_name(g))), None => bare.push(quote_name(&item.name)), + Some(g) => { + if !bare.is_empty() { + w!(out, "- {}", bare.join(", ")); + bare.clear(); + } + w!(out, "- {}[{}]", quote_name(&item.name), quote_name(g)); + } } } if !bare.is_empty() { w!(out, "- {}", bare.join(", ")); } - for g_item in &grouped { - w!(out, "- {g_item}"); - } for g in &cat.groups { if let Some(parent) = &g.parent { w!(out, "> {}[{}]", quote_name(&g.name), quote_name(parent)); @@ -190,7 +223,10 @@ pub fn format_md(workbook: &Workbook) -> String { CellValue::Number(n) => format_number(*n), CellValue::Text(s) | CellValue::Error(s) => pipe_quote(s), }; - w!(out, "{} = {}", coord_str(&key), val_str); + match coord_str(&key) { + coords if coords.is_empty() => w!(out, "= {val_str}"), + coords => w!(out, "{coords} = {val_str}"), + } } } diff --git a/crates/improvise-io/src/persistence/improv.pest b/crates/improvise-io/src/persistence/improv.pest index 8bacdb7c..094086a3 100644 --- a/crates/improvise-io/src/persistence/improv.pest +++ b/crates/improvise-io/src/persistence/improv.pest @@ -24,6 +24,7 @@ initial_view = { "Initial View: " ~ header_name ~ NEWLINE ~ blank_lines } section = _{ category_section | formulas_section + | aggregation_section | data_section | view_section } @@ -69,6 +70,19 @@ formula_line = { formula_target = _{ " "* ~ "[" ~ name ~ "]" } formula_raw = @{ (!(formula_target? ~ NEWLINE) ~ ANY)+ } +// ── Aggregation ────────────────────────────────────────────────────────────── +// +// How a `_Measure` item aggregates over the cells a grid position covers. +// Absent means `sum`, which is `Model::agg_for`'s fallback. + +aggregation_section = { + "## Aggregation" ~ NEWLINE ~ blank_lines ~ + aggregation_line* +} + +aggregation_line = { "- " ~ name ~ ": " ~ agg_kind ~ NEWLINE ~ blank_lines } +agg_kind = @{ "sum" | "avg" | "min" | "max" | "count" } + // ── Data ───────────────────────────────────────────────────────────────────── data_section = { @@ -76,11 +90,14 @@ data_section = { data_line* } +// A cell with NO coordinates is a legal model state — the scalar cell, and the +// placeholder an unfinished records row leaves. It is spelled `= 42`. data_line = { - coord_list ~ " = " ~ cell_value ~ NEWLINE ~ blank_lines + data_coords ~ "= " ~ cell_value ~ NEWLINE ~ blank_lines } -coord_list = { coord ~ (", " ~ coord)* } +data_coords = _{ (coord_list ~ " ")? } +coord_list = { coord ~ (", " ~ coord)* } coord = { name ~ "=" ~ name } cell_value = _{ number | pipe_quoted | bare_value } @@ -98,7 +115,7 @@ view_section = { view_entry* } -view_entry = _{ format_line | hidden_line | collapsed_line | axis_line } +view_entry = _{ format_line | prune_line | hidden_line | collapsed_line | axis_line } axis_line = { name ~ ": " ~ axis_kind ~ (", " ~ name)? ~ NEWLINE ~ blank_lines @@ -107,6 +124,10 @@ axis_line = { axis_kind = @{ "row" | "column" | "page" | "filter" | "none" } format_line = { "format: " ~ rest_of_line ~ NEWLINE ~ blank_lines } +// Absent means false — the compatible reading for a file written before it +// existed, and what the writer emits for a view that does not prune. +prune_line = { "prune: " ~ bool_kw ~ NEWLINE ~ blank_lines } +bool_kw = @{ "true" | "false" } hidden_line = { "hidden: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines } collapsed_line = { "collapsed: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines } diff --git a/crates/improvise-io/src/persistence/parse.rs b/crates/improvise-io/src/persistence/parse.rs index 156cbf90..963ed3b0 100644 --- a/crates/improvise-io/src/persistence/parse.rs +++ b/crates/improvise-io/src/persistence/parse.rs @@ -6,7 +6,7 @@ use pest::Parser; use pest::iterators::{Pair, Pairs}; use pest_derive::Parser; -use crate::formula::parse_formula; +use crate::formula::{AggFunc, parse_formula}; use crate::model::category::Group; use crate::model::cell::{CellKey, CellValue}; use crate::view::Axis; @@ -35,6 +35,7 @@ struct PView { axes: Vec<(String, Axis)>, page_selections: Vec<(String, String)>, format: String, + prune_empty: bool, hidden: Vec<(String, String)>, collapsed: Vec<(String, String)>, } @@ -46,6 +47,7 @@ struct ParsedFile { initial_view: Option, categories: Vec, formulas: Vec<(String, String)>, + measure_agg: Vec<(String, AggFunc)>, data: Vec<(CellKey, CellValue)>, views: Vec, } @@ -177,12 +179,39 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result { Rule::group_hierarchy => { pc.group_parents.push(extract_name_pair(entry)?); } - _ => {} + other => bail!("unhandled rule in category_section: {other:?}"), } } Ok(pc) } +/// The on-disk keyword for an aggregation, exhaustively — the reader's half of +/// `format.rs::agg_keyword`. +fn parse_agg_kind(kind: &str) -> Result { + match kind { + "sum" => Ok(AggFunc::Sum), + "avg" => Ok(AggFunc::Avg), + "min" => Ok(AggFunc::Min), + "max" => Ok(AggFunc::Max), + "count" => Ok(AggFunc::Count), + _ => bail!("Unknown aggregation: {kind}"), + } +} + +/// `## Aggregation`: `- Measure: avg` lines as `(item, AggFunc)` pairs. +fn walk_aggregation_section(pair: Pair<'_, Rule>) -> Result> { + let mut out = Vec::new(); + for line in pair.into_inner() { + if line.as_rule() == Rule::aggregation_line { + let mut parts = line.into_inner(); + let name = extract_name(next(&mut parts, "aggregation_line")?)?; + let kind = next(&mut parts, "aggregation_line")?; + out.push((name, parse_agg_kind(kind.as_str())?)); + } + } + Ok(out) +} + /// `## Formulas`: `(raw text, target category)` pairs. A line with no /// `[Category]` suffix targets `_Measure`. fn walk_formulas_section(pair: Pair<'_, Rule>) -> Result> { @@ -231,21 +260,28 @@ fn walk_data_section(pair: Pair<'_, Rule>) -> Result> for dl in pair.into_inner() { if dl.as_rule() == Rule::data_line { let mut dl_inner = dl.into_inner(); - let coord_list = next(&mut dl_inner, "data_line coords")?; - let value_pair = next(&mut dl_inner, "data_line value")?; - - let coords: Vec<_> = coord_list - .into_inner() - .filter(|p| p.as_rule() == Rule::coord) - .map(extract_name_pair) - .collect::>()?; - + let first = next(&mut dl_inner, "data_line")?; + // `coord_list` is optional in the grammar: a cell with no + // coordinates is the scalar cell, spelled `= 42` (improvise-pnf5). + let (coords, value_pair) = match first.as_rule() { + Rule::coord_list => (walk_coord_list(first)?, next(&mut dl_inner, "data_line")?), + _ => (Vec::new(), first), + }; out.push((CellKey::new(coords), walk_cell_value(value_pair)?)); } } Ok(out) } +/// The `Cat=Item, …` prefix of a data line. +fn walk_coord_list(coord_list: Pair<'_, Rule>) -> Result> { + coord_list + .into_inner() + .filter(|p| p.as_rule() == Rule::coord) + .map(extract_name_pair) + .collect() +} + /// The keyword after `Category:` on an axis line. fn parse_axis_kind(kind_str: &str) -> Result { match kind_str { @@ -281,6 +317,7 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result { axes: Vec::new(), page_selections: Vec::new(), format: String::new(), + prune_empty: false, hidden: Vec::new(), collapsed: Vec::new(), }; @@ -288,9 +325,10 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result { match entry.as_rule() { Rule::axis_line => walk_axis_line(entry, &mut pv)?, Rule::format_line => pv.format = first_str(entry)?, + Rule::prune_line => pv.prune_empty = first_str(entry)? == "true", Rule::hidden_line => pv.hidden.push(extract_name_pair(entry)?), Rule::collapsed_line => pv.collapsed.push(extract_name_pair(entry)?), - _ => {} + other => bail!("unhandled rule in view_section: {other:?}"), } } Ok(pv) @@ -310,9 +348,10 @@ fn walk_file(file: Pair<'_, Rule>) -> Result { } Rule::category_section => parsed.push_category(walk_category_section(pair)?), Rule::formulas_section => parsed.formulas.extend(walk_formulas_section(pair)?), + Rule::aggregation_section => parsed.measure_agg.extend(walk_aggregation_section(pair)?), Rule::data_section => parsed.data.extend(walk_data_section(pair)?), Rule::view_section => parsed.views.push(walk_view_section(pair)?), - _ => {} + other => bail!("unhandled rule in file: {other:?}"), } } Ok(parsed) @@ -367,6 +406,7 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> { if !pv.format.is_empty() { view.number_format = pv.format.clone(); } + view.prune_empty = pv.prune_empty; for (cat, item) in &pv.hidden { view.hide_item(cat, item); } @@ -376,6 +416,16 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> { Ok(()) } +/// `Workbook::new` seeds a `Default` view and `on_category_added` fills it, so +/// a file naming its own views gained a phantom one it never had (improvise-m4rv). +fn drop_unnamed_default(wb: &mut Workbook, named: &[&PView]) { + let default_is_a_phantom = + !named.is_empty() && !named.iter().any(|pv| pv.name == crate::view::DEFAULT_VIEW); + if default_is_a_phantom { + wb.delete_view(crate::view::DEFAULT_VIEW).ok(); + } +} + /// Turn pass 1's collectors into a [`Workbook`]. fn build_workbook(parsed: ParsedFile) -> Result { let name = parsed @@ -389,13 +439,19 @@ fn build_workbook(parsed: ParsedFile) -> Result { // A file written before improvise-60z may carry the scratch views; drop // them, and `initial_view` then falls through to the default. - for pv in parsed + let named: Vec<&PView> = parsed .views .iter() .filter(|pv| !crate::view::VIRTUAL_VIEW_NAMES.contains(&pv.name.as_str())) - { + .collect(); + for pv in &named { build_view(&mut wb, pv)?; } + drop_unnamed_default(&mut wb, &named); + + for (item, agg) in parsed.measure_agg { + wb.model.measure_agg.insert(item, agg); + } if let Some(iv) = &parsed.initial_view && wb.views.contains_key(iv) @@ -420,6 +476,55 @@ pub fn parse_md(text: &str) -> Result { build_workbook(walk_file(parse_tree(text)?)?) } +#[cfg(test)] +mod grammar_coverage { + use super::Rule; + + /// Which walker reads each grammar rule. A new rule in `improv.pest` adds a + /// `Rule` variant and this match stops compiling — improvise-4lgj. + fn walker_of(rule: Rule) -> &'static str { + match rule { + Rule::file | Rule::EOI => "walk_file", + Rule::version_line => "walk_file (skipped: the version is the literal)", + Rule::model_name | Rule::initial_view => "walk_file", + 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::category_entry => "silent: hoisted into category_section", + Rule::formulas_section | Rule::formula_line | Rule::formula_raw => { + "walk_formulas_section" + } + Rule::formula_target => "silent: hoisted into formula_line", + Rule::aggregation_section | Rule::aggregation_line | Rule::agg_kind => { + "walk_aggregation_section" + } + Rule::data_section | Rule::data_line => "walk_data_section", + Rule::data_coords => "silent: hoisted into data_line", + Rule::coord_list | Rule::coord => "walk_coord_list", + Rule::cell_value => "silent: hoisted into data_line", + Rule::number | Rule::bare_value => "walk_cell_value", + Rule::view_section => "walk_view_section", + Rule::axis_line | Rule::axis_kind => "walk_axis_line", + Rule::format_line | Rule::prune_line | Rule::bool_kw => "walk_view_section", + Rule::hidden_line | Rule::collapsed_line => "walk_view_section", + Rule::view_entry => "silent: hoisted into view_section", + Rule::name => "silent: hoisted into its parent", + Rule::pipe_quoted | Rule::pipe_inner | Rule::bare_name => "extract_name", + Rule::header_name | Rule::line_name => "header_name", + Rule::rest_of_line => "first_str", + Rule::blank_lines => "silent: whitespace", + } + } + + /// The compile is the test: this call only type-checks while every `Rule` + /// variant is classified above. The assertions are a smoke check. + #[test] + fn every_grammar_rule_names_the_walker_that_reads_it() { + assert_eq!(walker_of(Rule::prune_line), "walk_view_section"); + assert_eq!(walker_of(Rule::coord_list), "walk_coord_list"); + } +} + #[cfg(test)] mod tests { use super::parse_md; diff --git a/crates/improvise-io/src/persistence/roundtrip_tests.rs b/crates/improvise-io/src/persistence/roundtrip_tests.rs index 9f4f80e1..654af3f9 100644 --- a/crates/improvise-io/src/persistence/roundtrip_tests.rs +++ b/crates/improvise-io/src/persistence/roundtrip_tests.rs @@ -920,3 +920,204 @@ fn a_bare_item_name_with_a_space_is_rejected() { "precondition: the same line quoted must parse" ); } + +// ── prune_empty (improvise-qu4a) ───────────────────────────────────────── + +/// The FLAG is asserted, not the output's stability: both sides dropped it. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn prune_empty_survives_the_roundtrip() { + let mut wb = two_cat_model(); + wb.active_view_mut().prune_empty = true; + let text = format_md(&wb); + assert!(text.contains("prune: true"), "no prune line:\n{text}"); + assert!(parse_md(&text).unwrap().active_view().prune_empty); +} + +/// The direction the default supplies, which is why it is the discriminating +/// one to state separately: a writer that always emits `prune: true` fails it. +#[test] +fn a_view_that_does_not_prune_writes_no_prune_line() { + let mut wb = two_cat_model(); + wb.active_view_mut().prune_empty = false; + let text = format_md(&wb); + assert!(!text.contains("prune:"), "unexpected prune line:\n{text}"); + assert!(!parse_md(&text).unwrap().active_view().prune_empty); +} + +/// An older file carries no `prune:` line; `false` is the compatible reading. +#[test] +fn a_file_without_a_prune_line_does_not_prune() { + let text = "v2025-04-09\n# T\n\n## View: Default\n\n## Category: C\n- a\n"; + assert!(!parse_md(text).unwrap().active_view().prune_empty); +} + +// ── The seeded Default view (improvise-m4rv) ───────────────────────────── + +/// A file that names its own views must not gain the one `Workbook::new` +/// seeds. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_file_naming_its_own_views_gains_no_default() { + let text = "v2025-04-09\n# T\nInitial View: Plan\n\n## View: Plan\n\n## Category: C\n- a\n"; + let wb = parse_md(text).unwrap(); + assert_eq!(wb.views.keys().collect::>(), vec!["Plan"]); + assert_eq!(wb.active_view, "Plan"); +} + +/// The control the fix must not break, and the SECOND view is what makes it +/// discriminating. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_file_that_names_default_itself_keeps_it_alongside_its_siblings() { + let text = "v2025-04-09\n# T\nInitial View: Default\n\n## View: Default\n\n\ + ## View: Other\n\n## Category: C\n- a\n"; + let wb = parse_md(text).unwrap(); + assert_eq!( + wb.views.keys().collect::>(), + vec!["Default", "Other"] + ); + assert_eq!(wb.active_view, "Default"); +} + +/// The legacy reading: a file with no `## View` section at all still opens on +/// the seeded view. Weaker than its neighbour on purpose — see the evidence. +#[test] +fn a_file_naming_no_view_keeps_the_seeded_default() { + let text = "v2025-04-09\n# T\n\n## Category: C\n- a\n"; + let wb = parse_md(text).unwrap(); + assert_eq!(wb.views.keys().collect::>(), vec!["Default"]); +} + +/// A category named `prune` writes `prune: row`, which the reader must take as +/// an axis line. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_category_named_prune_is_not_read_as_the_prune_flag() { + let mut wb = Workbook::new("T"); + wb.add_category("prune").unwrap(); + wb.add_item("prune", "x").unwrap(); + wb.active_view_mut().prune_empty = true; + // Page, NOT the Row `on_category_added` would re-derive: a swallowed axis + // line is otherwise invisible, because reloading reinvents the default. + wb.active_view_mut().set_axis("prune", Axis::Page); + wb.active_view_mut().set_page_selection("prune", "x"); + let text = format_md(&wb); + let back = parse_md(&text).unwrap_or_else(|e| panic!("unparseable: {e}\n{text}")); + assert!(back.active_view().prune_empty, "the flag was lost:\n{text}"); + assert_eq!( + back.active_view().axis_of("prune"), + Axis::Page, + "the axis line was swallowed by the prune line:\n{text}" + ); + assert_eq!(back.active_view().page_selection("prune"), Some("x")); +} + +/// improvise-ppru made this durable, so the sweep had to come with it. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_removed_measures_aggregation_is_not_written() { + let mut wb = Workbook::new("T"); + for name in ["Price", "Volume"] { + wb.add_item("_Measure", name).unwrap(); + wb.model + .measure_agg + .insert(name.into(), crate::formula::AggFunc::Avg); + } + wb.remove_item("_Measure", "Price"); + let text = format_md(&wb); + assert!( + !text.contains("- Price:"), + "Price outlived its item:\n{text}" + ); + let back = parse_md(&text).unwrap(); + assert!(!back.model.measure_agg.contains_key("Price")); + assert!( + matches!( + back.model.measure_agg.get("Volume"), + Some(crate::formula::AggFunc::Avg) + ), + "the sweep must take one measure, not the map: {:?}", + back.model.measure_agg + ); +} + +// ── Aggregation (improvise-ppru) ───────────────────────────────────────── + +/// Two saves of one document must agree: `measure_agg` is a `HashMap`, and +/// hash order is not an order (gotcha 18). +#[test] +fn the_aggregation_section_is_written_in_a_stable_order() { + let mut wb = Workbook::new("T"); + for (item, agg) in [ + ("Zebra", crate::formula::AggFunc::Min), + ("Apple", crate::formula::AggFunc::Count), + ("Mango", crate::formula::AggFunc::Avg), + ] { + wb.add_item("_Measure", item).unwrap(); + wb.model.measure_agg.insert(item.into(), agg); + } + let text = format_md(&wb); + let lines: Vec<&str> = text + .lines() + .skip_while(|l| *l != "## Aggregation") + .skip(1) + .take(3) + .collect(); + assert_eq!( + lines, + vec!["- Apple: count", "- Mango: avg", "- Zebra: min"] + ); + assert_eq!(format_md(&parse_md(&text).unwrap()), text); +} + +// ── Interleaved items (improvise-3yv3) ─────────────────────────────────── + +/// The writer emits items in insertion order, splitting a bare run where a +/// grouped item interrupts it. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn an_interleaved_category_keeps_its_item_order() { + let mut wb = Workbook::new("T"); + wb.add_category("C").unwrap(); + for name in ["a", "b"] { + wb.add_item("C", name).unwrap(); + } + wb.add_item_in_group("C", "g1", "G").unwrap(); + wb.add_item("C", "c").unwrap(); + wb.add_item_in_group("C", "g2", "G").unwrap(); + + let text = format_md(&wb); + let body: Vec<&str> = text + .lines() + .skip_while(|l| *l != "## Category: C") + .skip(1) + .take(4) + .collect(); + assert_eq!(body, vec!["- a, b", "- g1[G]", "- c", "- g2[G]"]); + assert_eq!( + parse_md(&text) + .unwrap() + .model + .category("C") + .unwrap() + .ordered_item_names(), + wb.model.category("C").unwrap().ordered_item_names() + ); +} + +// ── The scalar cell (improvise-pnf5) ───────────────────────────────────── + +/// A cell with no coordinates is a legal model state and now has a spelling. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_cell_with_no_coordinates_round_trips() { + use crate::model::cell::CellKey; + let mut wb = two_cat_model(); + wb.model + .set_cell(CellKey::new(vec![]), CellValue::Number(42.0)); + let text = format_md(&wb); + assert!(text.contains("\n= 42\n"), "expected a scalar line:\n{text}"); + let back = parse_md(&text).unwrap(); + assert_eq!( + back.model.data.get(&CellKey::new(vec![])), + Some(&CellValue::Number(42.0)) + ); + assert_eq!(format_md(&back), text); +} diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index fc4c5f70..8da1d071 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -73,7 +73,7 @@ test.describe("every field the render model projects reaches the screen", () => // The axis line is the shared `view_axis_summary`; its absence would mean // a second derivation had crept in. await expect(page.locator("#views li").nth(0).locator(".axes")).toHaveText( - "R:Product C:Quarter", + "R:Product P:Quarter", ); await page.keyboard.press("Escape"); @@ -196,21 +196,21 @@ test.describe("a pointer event reaches the client and its answer is painted", () // A view row (improvise-iioi). The activation must read the CLICKED index, // not `view_panel_cursor` — so the cursor must not already be on the row - // clicked, or both readings give the same answer. `Default` is index 0 and - // holds the cursor by default, so move to it and click `Plan`. + // clicked, or both readings give the same answer. `Plan` is index 0 and + // holds the cursor by default, so move to it and click `Season`. await page.keyboard.press("Shift+V"); - await expect(page.locator("#views li.selected")).toContainText("Default"); - // Switch to Default by KEYBOARD, leaving the cursor on it. The click then + await expect(page.locator("#views li.selected")).toContainText("Plan"); + // Switch to Plan by KEYBOARD, leaving the cursor on it. The click then // names a different row than the cursor does, which is the whole point. await page.keyboard.press("Enter"); - await expect(page.locator("#view-name")).toHaveText("Default"); - - const plan = page.locator("#views li").filter({ hasText: "Plan" }).first(); - await plan.click(); - // Reading the cursor instead of the index re-selects Default and fails. await expect(page.locator("#view-name")).toHaveText("Plan"); - await expect(page.locator("#views li.active")).toContainText("Plan"); - await expect(page.locator("#views li.selected")).toContainText("Plan"); + + const season = page.locator("#views li").filter({ hasText: "Season" }).first(); + await season.click(); + // Reading the cursor instead of the index re-selects Plan and fails. + await expect(page.locator("#view-name")).toHaveText("Season"); + await expect(page.locator("#views li.active")).toContainText("Season"); + await expect(page.locator("#views li.selected")).toContainText("Season"); // A formula row: navigation only, because FormulaPanel binds no Enter and // a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md. diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index e945d3cc..7fc16442 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -8,7 +8,6 @@ use improvise_io::{ workbook::Workbook, }; #[test] -#[ignore = "improvise-3yv3: known bug reproduction"] fn grouped_item_order_survives_save() { let mut w = Workbook::new("test"); w.add_category("C").unwrap(); @@ -21,7 +20,6 @@ fn grouped_item_order_survives_save() { ); } #[test] -#[ignore = "improvise-ppru: known bug reproduction"] fn aggregation_survives_save() { let mut w = Workbook::new("test"); w.add_item("_Measure", "Price").unwrap(); @@ -34,7 +32,6 @@ fn aggregation_survives_save() { ); } #[test] -#[ignore = "improvise-m4rv: known bug reproduction"] fn deleted_default_stays_deleted() { let mut w = Workbook::new("test"); w.create_view("Custom"); @@ -208,7 +205,6 @@ fn literal_cr_name_survives_roundtrip() { ); } #[test] -#[ignore = "improvise-pnf5: known bug reproduction"] fn scalar_cell_survives_roundtrip() { use improvise_io::model::cell::{CellKey, CellValue}; let mut w = Workbook::new("test"); @@ -317,7 +313,6 @@ fn formula_panel_handles_zero_inner_width() { } #[test] -#[ignore = "improvise-pnf5: known bug reproduction"] fn save_with_unfinished_record_is_readable() { use improvise::{ command::{ @@ -426,3 +421,25 @@ fn ordinary_controls() { ); assert!(parse_md(&format_md(&w)).is_ok()); } + +// Passing control (improvise-nvq4): quote_name already pipe-quotes a name +// containing `/`, so hidden/collapsed round-trip. The report did not reproduce. +#[test] +fn slash_in_hidden_name_survives_roundtrip() { + let mut w = Workbook::new("test"); + w.add_category("C").unwrap(); + w.add_item("C", "A/B").unwrap(); + w.add_item_in_group("C", "x", "G/H").unwrap(); + w.active_view_mut().hide_item("C", "A/B"); + w.active_view_mut().toggle_group_collapse("C", "G/H"); + let text = format_md(&w); + let r = parse_md(&text).unwrap_or_else(|e| panic!("unparseable: {e}\n{text}")); + assert!( + r.active_view().is_hidden("C", "A/B"), + "hidden lost:\n{text}" + ); + assert!( + r.active_view().is_group_collapsed("C", "G/H"), + "collapsed lost:\n{text}" + ); +} diff --git a/web/demo.improv b/web/demo.improv index b2eb845b..b1e14a28 100644 --- a/web/demo.improv +++ b/web/demo.improv @@ -10,6 +10,14 @@ _Index: none _Dim: none format: ,.0 +## View: Season +Quarter: row +_Measure: column +Product: page, Latte +_Index: none +_Dim: none +format: ,.0 + ## Formulas - Profit = Revenue - Cost