fix(io): five .improv persistence defects, grammar first
improvise-pnf5 (P1). A cell keyed by the empty CellKey is a legal model state -- what AddRecordRow leaves while a records row is unfinished -- and format_md wrote it as " = 0" while coord_list required at least one coordinate. Saving in records mode therefore overwrote a good document with one the application could not reopen. Given a spelling rather than discarded, because the ticket is explicit that legitimate data must not be dropped: data_coords makes the coordinate list optional and the scalar cell is written "= 42". Unambiguous under PEG, since bare_name cannot start with "=". improvise-m4rv. Workbook::new seeds a Default view and on_category_added fills it, so a file naming only Plan parsed into two views -- the phantom at index 0, and appended to the user's document on any save. parse_md now drops Default when the file named at least one view and none was Default. A file naming no view, or naming Default itself, is untouched. improvise-3yv3. format_md collected every ungrouped item and emitted the whole list before any grouped one, so an interleaved category came back reordered -- and grid axis ordering consumes item insertion order (gotcha 18). Items are now written in insertion order, a run of bare items sharing a line. improvise-qu4a. View::prune_empty never reached disk. The line is spelled with a value rather than by its presence, so the grammar generator can emit either; absent means false, the compatible reading. improvise-ppru. Model::measure_agg was lost by the roundtrip. An Aggregation section carries it, sorted -- measure_agg is a HashMap and hash order is not an order (gotcha 18), so two saves of one document must be made to agree. improvise-4lgj. The three wildcard arms in the walker silently skipped any grammar rule they did not handle, so extending improv.pest caused silent data loss rather than a loud mismatch. They now bail, and a new exhaustive match over Rule names the walker that reads each one -- a new rule stops that test compiling, which is design-principles section 2's "the compiler finds every call site" applied to a pest-derived enum. It earned its keep immediately by naming Rule::section, which the first draft had missed. That arm is also what makes the change safe: the grammar-walking generator reaches both new rules -- proved by injecting a bail into each and watching generated_file_parses fail -- so an unwalked rule is now a test failure. Un-ignores the five reproductions in tests/bug_hunt_reproductions.rs. improvise-nvq4 did NOT reproduce and is kept as a passing control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -7,6 +7,7 @@ use crate::workbook::Workbook;
|
|||||||
|
|
||||||
// The one home of the pipe-quoting rule (improvise-i2m5); a name written here
|
// 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`.
|
// must also survive `parse_formula`. See `context/code/formula-and-io.md`.
|
||||||
|
use crate::formula::AggFunc;
|
||||||
use crate::formula::quote::{pipe_quote, quote_name};
|
use crate::formula::quote::{pipe_quote, quote_name};
|
||||||
|
|
||||||
/// A name as a section header spells it: the whole line, so no pipes where a
|
/// 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 {
|
fn format_number(n: f64) -> String {
|
||||||
if n.is_infinite() {
|
if n.is_infinite() {
|
||||||
return if n.is_sign_positive() {
|
return if n.is_sign_positive() {
|
||||||
@@ -108,6 +121,9 @@ pub fn format_md(workbook: &Workbook) -> String {
|
|||||||
if !view.number_format.is_empty() {
|
if !view.number_format.is_empty() {
|
||||||
w!(out, "format: {}", view.number_format);
|
w!(out, "format: {}", view.number_format);
|
||||||
}
|
}
|
||||||
|
if view.prune_empty {
|
||||||
|
w!(out, "prune: true");
|
||||||
|
}
|
||||||
for (prefix, map) in [
|
for (prefix, map) in [
|
||||||
("hidden", &view.hidden_items),
|
("hidden", &view.hidden_items),
|
||||||
("collapsed", &view.collapsed_groups),
|
("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) ───────────────
|
// ── Categories (items comma-separated on one line) ───────────────
|
||||||
// Collect formula targets so we can exclude them from _Measure items
|
// Collect formula targets so we can exclude them from _Measure items
|
||||||
let formula_targets: std::collections::HashSet<&str> = model
|
let formula_targets: std::collections::HashSet<&str> = model
|
||||||
@@ -152,8 +181,9 @@ pub fn format_md(workbook: &Workbook) -> String {
|
|||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
w!(out, "\n## Category: {}", quote_header(&cat.name));
|
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 bare: Vec<String> = Vec::new();
|
||||||
let mut grouped: Vec<String> = Vec::new();
|
|
||||||
for item in cat.items.values() {
|
for item in cat.items.values() {
|
||||||
// For _Measure, skip items that are formula targets
|
// For _Measure, skip items that are formula targets
|
||||||
// (they'll be recreated from the ## Formulas section)
|
// (they'll be recreated from the ## Formulas section)
|
||||||
@@ -163,16 +193,19 @@ pub fn format_md(workbook: &Workbook) -> String {
|
|||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
match &item.group {
|
match &item.group {
|
||||||
Some(g) => grouped.push(format!("{}[{}]", quote_name(&item.name), quote_name(g))),
|
|
||||||
None => bare.push(quote_name(&item.name)),
|
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() {
|
if !bare.is_empty() {
|
||||||
w!(out, "- {}", bare.join(", "));
|
w!(out, "- {}", bare.join(", "));
|
||||||
}
|
}
|
||||||
for g_item in &grouped {
|
|
||||||
w!(out, "- {g_item}");
|
|
||||||
}
|
|
||||||
for g in &cat.groups {
|
for g in &cat.groups {
|
||||||
if let Some(parent) = &g.parent {
|
if let Some(parent) = &g.parent {
|
||||||
w!(out, "> {}[{}]", quote_name(&g.name), quote_name(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::Number(n) => format_number(*n),
|
||||||
CellValue::Text(s) | CellValue::Error(s) => pipe_quote(s),
|
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 = _{
|
section = _{
|
||||||
category_section
|
category_section
|
||||||
| formulas_section
|
| formulas_section
|
||||||
|
| aggregation_section
|
||||||
| data_section
|
| data_section
|
||||||
| view_section
|
| view_section
|
||||||
}
|
}
|
||||||
@@ -69,6 +70,19 @@ formula_line = {
|
|||||||
formula_target = _{ " "* ~ "[" ~ name ~ "]" }
|
formula_target = _{ " "* ~ "[" ~ name ~ "]" }
|
||||||
formula_raw = @{ (!(formula_target? ~ NEWLINE) ~ ANY)+ }
|
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 ─────────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
data_section = {
|
data_section = {
|
||||||
@@ -76,11 +90,14 @@ data_section = {
|
|||||||
data_line*
|
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 = {
|
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 }
|
coord = { name ~ "=" ~ name }
|
||||||
|
|
||||||
cell_value = _{ number | pipe_quoted | bare_value }
|
cell_value = _{ number | pipe_quoted | bare_value }
|
||||||
@@ -98,7 +115,7 @@ view_section = {
|
|||||||
view_entry*
|
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 = {
|
axis_line = {
|
||||||
name ~ ": " ~ axis_kind ~ (", " ~ name)? ~ NEWLINE ~ blank_lines
|
name ~ ": " ~ axis_kind ~ (", " ~ name)? ~ NEWLINE ~ blank_lines
|
||||||
@@ -107,6 +124,10 @@ axis_line = {
|
|||||||
axis_kind = @{ "row" | "column" | "page" | "filter" | "none" }
|
axis_kind = @{ "row" | "column" | "page" | "filter" | "none" }
|
||||||
|
|
||||||
format_line = { "format: " ~ rest_of_line ~ NEWLINE ~ blank_lines }
|
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 }
|
hidden_line = { "hidden: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines }
|
||||||
collapsed_line = { "collapsed: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines }
|
collapsed_line = { "collapsed: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines }
|
||||||
|
|
||||||
|
|||||||
@@ -6,7 +6,7 @@ use pest::Parser;
|
|||||||
use pest::iterators::{Pair, Pairs};
|
use pest::iterators::{Pair, Pairs};
|
||||||
use pest_derive::Parser;
|
use pest_derive::Parser;
|
||||||
|
|
||||||
use crate::formula::parse_formula;
|
use crate::formula::{AggFunc, parse_formula};
|
||||||
use crate::model::category::Group;
|
use crate::model::category::Group;
|
||||||
use crate::model::cell::{CellKey, CellValue};
|
use crate::model::cell::{CellKey, CellValue};
|
||||||
use crate::view::Axis;
|
use crate::view::Axis;
|
||||||
@@ -35,6 +35,7 @@ struct PView {
|
|||||||
axes: Vec<(String, Axis)>,
|
axes: Vec<(String, Axis)>,
|
||||||
page_selections: Vec<(String, String)>,
|
page_selections: Vec<(String, String)>,
|
||||||
format: String,
|
format: String,
|
||||||
|
prune_empty: bool,
|
||||||
hidden: Vec<(String, String)>,
|
hidden: Vec<(String, String)>,
|
||||||
collapsed: Vec<(String, String)>,
|
collapsed: Vec<(String, String)>,
|
||||||
}
|
}
|
||||||
@@ -46,6 +47,7 @@ struct ParsedFile {
|
|||||||
initial_view: Option<String>,
|
initial_view: Option<String>,
|
||||||
categories: Vec<PCategory>,
|
categories: Vec<PCategory>,
|
||||||
formulas: Vec<(String, String)>,
|
formulas: Vec<(String, String)>,
|
||||||
|
measure_agg: Vec<(String, AggFunc)>,
|
||||||
data: Vec<(CellKey, CellValue)>,
|
data: Vec<(CellKey, CellValue)>,
|
||||||
views: Vec<PView>,
|
views: Vec<PView>,
|
||||||
}
|
}
|
||||||
@@ -177,12 +179,39 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result<PCategory> {
|
|||||||
Rule::group_hierarchy => {
|
Rule::group_hierarchy => {
|
||||||
pc.group_parents.push(extract_name_pair(entry)?);
|
pc.group_parents.push(extract_name_pair(entry)?);
|
||||||
}
|
}
|
||||||
_ => {}
|
other => bail!("unhandled rule in category_section: {other:?}"),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok(pc)
|
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
|
/// `## Formulas`: `(raw text, target category)` pairs. A line with no
|
||||||
/// `[Category]` suffix targets `_Measure`.
|
/// `[Category]` suffix targets `_Measure`.
|
||||||
fn walk_formulas_section(pair: Pair<'_, Rule>) -> Result<Vec<(String, String)>> {
|
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() {
|
for dl in pair.into_inner() {
|
||||||
if dl.as_rule() == Rule::data_line {
|
if dl.as_rule() == Rule::data_line {
|
||||||
let mut dl_inner = dl.into_inner();
|
let mut dl_inner = dl.into_inner();
|
||||||
let coord_list = next(&mut dl_inner, "data_line coords")?;
|
let first = next(&mut dl_inner, "data_line")?;
|
||||||
let value_pair = next(&mut dl_inner, "data_line value")?;
|
// `coord_list` is optional in the grammar: a cell with no
|
||||||
|
// coordinates is the scalar cell, spelled `= 42` (improvise-pnf5).
|
||||||
let coords: Vec<_> = coord_list
|
let (coords, value_pair) = match first.as_rule() {
|
||||||
.into_inner()
|
Rule::coord_list => (walk_coord_list(first)?, next(&mut dl_inner, "data_line")?),
|
||||||
.filter(|p| p.as_rule() == Rule::coord)
|
_ => (Vec::new(), first),
|
||||||
.map(extract_name_pair)
|
};
|
||||||
.collect::<Result<_>>()?;
|
|
||||||
|
|
||||||
out.push((CellKey::new(coords), walk_cell_value(value_pair)?));
|
out.push((CellKey::new(coords), walk_cell_value(value_pair)?));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok(out)
|
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.
|
/// The keyword after `Category:` on an axis line.
|
||||||
fn parse_axis_kind(kind_str: &str) -> Result<Axis> {
|
fn parse_axis_kind(kind_str: &str) -> Result<Axis> {
|
||||||
match kind_str {
|
match kind_str {
|
||||||
@@ -281,6 +317,7 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result<PView> {
|
|||||||
axes: Vec::new(),
|
axes: Vec::new(),
|
||||||
page_selections: Vec::new(),
|
page_selections: Vec::new(),
|
||||||
format: String::new(),
|
format: String::new(),
|
||||||
|
prune_empty: false,
|
||||||
hidden: Vec::new(),
|
hidden: Vec::new(),
|
||||||
collapsed: Vec::new(),
|
collapsed: Vec::new(),
|
||||||
};
|
};
|
||||||
@@ -288,9 +325,10 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result<PView> {
|
|||||||
match entry.as_rule() {
|
match entry.as_rule() {
|
||||||
Rule::axis_line => walk_axis_line(entry, &mut pv)?,
|
Rule::axis_line => walk_axis_line(entry, &mut pv)?,
|
||||||
Rule::format_line => pv.format = first_str(entry)?,
|
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::hidden_line => pv.hidden.push(extract_name_pair(entry)?),
|
||||||
Rule::collapsed_line => pv.collapsed.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)
|
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::category_section => parsed.push_category(walk_category_section(pair)?),
|
||||||
Rule::formulas_section => parsed.formulas.extend(walk_formulas_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::data_section => parsed.data.extend(walk_data_section(pair)?),
|
||||||
Rule::view_section => parsed.views.push(walk_view_section(pair)?),
|
Rule::view_section => parsed.views.push(walk_view_section(pair)?),
|
||||||
_ => {}
|
other => bail!("unhandled rule in file: {other:?}"),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok(parsed)
|
Ok(parsed)
|
||||||
@@ -367,6 +406,7 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> {
|
|||||||
if !pv.format.is_empty() {
|
if !pv.format.is_empty() {
|
||||||
view.number_format = pv.format.clone();
|
view.number_format = pv.format.clone();
|
||||||
}
|
}
|
||||||
|
view.prune_empty = pv.prune_empty;
|
||||||
for (cat, item) in &pv.hidden {
|
for (cat, item) in &pv.hidden {
|
||||||
view.hide_item(cat, item);
|
view.hide_item(cat, item);
|
||||||
}
|
}
|
||||||
@@ -376,6 +416,16 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> {
|
|||||||
Ok(())
|
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`].
|
/// Turn pass 1's collectors into a [`Workbook`].
|
||||||
fn build_workbook(parsed: ParsedFile) -> Result<Workbook> {
|
fn build_workbook(parsed: ParsedFile) -> Result<Workbook> {
|
||||||
let name = parsed
|
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
|
// A file written before improvise-60z may carry the scratch views; drop
|
||||||
// them, and `initial_view` then falls through to the default.
|
// them, and `initial_view` then falls through to the default.
|
||||||
for pv in parsed
|
let named: Vec<&PView> = parsed
|
||||||
.views
|
.views
|
||||||
.iter()
|
.iter()
|
||||||
.filter(|pv| !crate::view::VIRTUAL_VIEW_NAMES.contains(&pv.name.as_str()))
|
.filter(|pv| !crate::view::VIRTUAL_VIEW_NAMES.contains(&pv.name.as_str()))
|
||||||
{
|
.collect();
|
||||||
|
for pv in &named {
|
||||||
build_view(&mut wb, pv)?;
|
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
|
if let Some(iv) = &parsed.initial_view
|
||||||
&& wb.views.contains_key(iv)
|
&& wb.views.contains_key(iv)
|
||||||
@@ -420,6 +476,55 @@ pub fn parse_md(text: &str) -> Result<Workbook> {
|
|||||||
build_workbook(walk_file(parse_tree(text)?)?)
|
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)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::parse_md;
|
use super::parse_md;
|
||||||
|
|||||||
@@ -920,3 +920,144 @@ fn a_bare_item_name_with_a_space_is_rejected() {
|
|||||||
"precondition: the same line quoted must parse"
|
"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: a file naming NO view still opens on
|
||||||
|
/// the seeded one, and a file that names `Default` itself keeps it.
|
||||||
|
#[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"]);
|
||||||
|
|
||||||
|
let named = "v2025-04-09\n# T\n\n## View: Default\n\n## Category: C\n- a\n";
|
||||||
|
assert_eq!(
|
||||||
|
parse_md(named).unwrap().views.keys().collect::<Vec<_>>(),
|
||||||
|
vec!["Default"]
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// ── 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);
|
||||||
|
}
|
||||||
|
|||||||
@@ -8,7 +8,6 @@ use improvise_io::{
|
|||||||
workbook::Workbook,
|
workbook::Workbook,
|
||||||
};
|
};
|
||||||
#[test]
|
#[test]
|
||||||
#[ignore = "improvise-3yv3: known bug reproduction"]
|
|
||||||
fn grouped_item_order_survives_save() {
|
fn grouped_item_order_survives_save() {
|
||||||
let mut w = Workbook::new("test");
|
let mut w = Workbook::new("test");
|
||||||
w.add_category("C").unwrap();
|
w.add_category("C").unwrap();
|
||||||
@@ -21,7 +20,6 @@ fn grouped_item_order_survives_save() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
#[test]
|
#[test]
|
||||||
#[ignore = "improvise-ppru: known bug reproduction"]
|
|
||||||
fn aggregation_survives_save() {
|
fn aggregation_survives_save() {
|
||||||
let mut w = Workbook::new("test");
|
let mut w = Workbook::new("test");
|
||||||
w.add_item("_Measure", "Price").unwrap();
|
w.add_item("_Measure", "Price").unwrap();
|
||||||
@@ -34,7 +32,6 @@ fn aggregation_survives_save() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
#[test]
|
#[test]
|
||||||
#[ignore = "improvise-m4rv: known bug reproduction"]
|
|
||||||
fn deleted_default_stays_deleted() {
|
fn deleted_default_stays_deleted() {
|
||||||
let mut w = Workbook::new("test");
|
let mut w = Workbook::new("test");
|
||||||
w.create_view("Custom");
|
w.create_view("Custom");
|
||||||
@@ -203,7 +200,6 @@ fn literal_cr_name_survives_roundtrip() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
#[test]
|
#[test]
|
||||||
#[ignore = "improvise-pnf5: known bug reproduction"]
|
|
||||||
fn scalar_cell_survives_roundtrip() {
|
fn scalar_cell_survives_roundtrip() {
|
||||||
use improvise_io::model::cell::{CellKey, CellValue};
|
use improvise_io::model::cell::{CellKey, CellValue};
|
||||||
let mut w = Workbook::new("test");
|
let mut w = Workbook::new("test");
|
||||||
@@ -314,7 +310,6 @@ fn formula_panel_handles_zero_inner_width() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
#[ignore = "improvise-pnf5: known bug reproduction"]
|
|
||||||
fn save_with_unfinished_record_is_readable() {
|
fn save_with_unfinished_record_is_readable() {
|
||||||
use improvise::{
|
use improvise::{
|
||||||
command::{
|
command::{
|
||||||
@@ -417,3 +412,25 @@ fn ordinary_controls() {
|
|||||||
);
|
);
|
||||||
assert!(parse_md(&format_md(&w)).is_ok());
|
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}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user