Merge pull request #108 from fiddlerwoaroof/wave/j-persistence

wave/j: .improv persistence — eight beads, and the format changed
This commit is contained in:
Ed Langley
2026-09-21 16:23:57 -07:00
committed by GitHub
13 changed files with 729 additions and 45 deletions
+35
View File
@@ -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
+164
View File
@@ -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`
+5
View File
@@ -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<CellKey> = self
.data
.iter_cells()
+1 -1
View File
@@ -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};
+68
View File
@@ -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");
+27 -3
View File
@@ -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<String>) -> 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");
+42 -6
View File
@@ -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<String> = Vec::new();
let mut grouped: Vec<String> = 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}"),
}
}
}
@@ -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 }
+120 -15
View File
@@ -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<String>,
categories: Vec<PCategory>,
formulas: Vec<(String, String)>,
measure_agg: Vec<(String, AggFunc)>,
data: Vec<(CellKey, CellValue)>,
views: Vec<PView>,
}
@@ -177,12 +179,39 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result<PCategory> {
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<AggFunc> {
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<Vec<(String, AggFunc)>> {
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<Vec<(String, String)>> {
@@ -231,21 +260,28 @@ fn walk_data_section(pair: Pair<'_, Rule>) -> Result<Vec<(CellKey, CellValue)>>
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::<Result<_>>()?;
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<Vec<(String, String)>> {
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<Axis> {
match kind_str {
@@ -281,6 +317,7 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result<PView> {
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<PView> {
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<ParsedFile> {
}
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<Workbook> {
let name = parsed
@@ -389,13 +439,19 @@ fn build_workbook(parsed: ParsedFile) -> Result<Workbook> {
// 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<Workbook> {
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;
@@ -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<_>>(), 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<_>>(),
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<_>>(), 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);
}
+12 -12
View File
@@ -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.
+22 -5
View File
@@ -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}"
);
}
+8
View File
@@ -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