From 1345142fe04bc6a2793c06688df6d40e90b5e10d Mon Sep 17 00:00:00 2001 From: Ed L Date: Tue, 24 Mar 2026 11:37:40 -0700 Subject: [PATCH] fix: make .improv parser order-independent via two-pass approach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: set_axis silently ignores unregistered categories, so a view section appearing before its categories would produce wrong axis assignments when on_category_added later ran and assigned defaults. Fix: collect all raw data in pass 1, then build the model in the correct dependency order in pass 2 (categories → views → data/formulas). The file can now list sections in any order. Co-Authored-By: Claude Sonnet 4.6 --- src/persistence/mod.rs | 338 +++++++++++++++++++++++++---------------- 1 file changed, 210 insertions(+), 128 deletions(-) diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index 1bf38cb..20fe5fa 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -144,170 +144,193 @@ pub fn format_md(model: &Model) -> String { } /// Parse the markdown `.improv` format into a Model. +/// +/// Uses a two-pass approach so the file is order-independent: +/// pass 1 collects raw data, pass 2 builds the model with categories +/// registered before views are configured. pub fn parse_md(text: &str) -> Result { + // ── Intermediate types ──────────────────────────────────────────────────── + + struct PCategory { + name: String, + items: Vec<(String, Option)>, // (name, group) + group_parents: Vec<(String, String)>, // (group, parent) + } + + struct PView { + name: String, + is_active: bool, + axes: Vec<(String, Axis)>, + page_selections: Vec<(String, String)>, + format: String, + hidden: Vec<(String, String)>, + collapsed: Vec<(String, String)>, + } + + // ── Pass 1: collect ─────────────────────────────────────────────────────── + #[derive(PartialEq)] enum Section { None, Category, Formulas, Data, View } - let mut model: Option = None; - let mut current_cat = String::new(); - let mut current_view = String::new(); - let mut active_view_name = String::new(); + let mut model_name: Option = None; + let mut categories: Vec = Vec::new(); + let mut formulas: Vec<(String, String)> = Vec::new(); // (raw, category) + let mut data: Vec<(CellKey, CellValue)> = Vec::new(); + let mut views: Vec = Vec::new(); let mut section = Section::None; for line in text.lines() { let trimmed = line.trim(); if trimmed.is_empty() { continue; } - // Model title if trimmed.starts_with("# ") && !trimmed.starts_with("## ") { - model = Some(Model::new(trimmed[2..].trim())); + model_name = Some(trimmed[2..].trim().to_string()); continue; } - - let m = match model.as_mut() { - Some(m) => m, - None => continue, - }; - - // Section headers if let Some(rest) = trimmed.strip_prefix("## Category: ") { - current_cat = rest.trim().to_string(); - m.add_category(¤t_cat)?; + categories.push(PCategory { name: rest.trim().to_string(), + items: Vec::new(), group_parents: Vec::new() }); section = Section::Category; continue; } - if trimmed == "## Formulas" { - section = Section::Formulas; - continue; - } - if trimmed == "## Data" { - section = Section::Data; - continue; - } + if trimmed == "## Formulas" { section = Section::Formulas; continue; } + if trimmed == "## Data" { section = Section::Data; continue; } if let Some(rest) = trimmed.strip_prefix("## View: ") { let (name, is_active) = match rest.trim().strip_suffix(" (active)") { Some(n) => (n.trim().to_string(), true), None => (rest.trim().to_string(), false), }; - if is_active { active_view_name = name.clone(); } - if !m.views.contains_key(&name) { m.create_view(&name); } - current_view = name; + views.push(PView { name, is_active, axes: Vec::new(), + page_selections: Vec::new(), format: String::new(), + hidden: Vec::new(), collapsed: Vec::new() }); section = Section::View; continue; } if trimmed.starts_with("## ") { continue; } match section { - Section::Category => parse_category_line(trimmed, ¤t_cat, m), - Section::Formulas => parse_formula_line(trimmed, m)?, - Section::Data => parse_data_line(trimmed, m), - Section::View => parse_view_line(trimmed, ¤t_view, m)?, - Section::None => {} - } - } - - let mut m = model.ok_or_else(|| anyhow::anyhow!("Empty or invalid .improv file"))?; - if !active_view_name.is_empty() && m.views.contains_key(&active_view_name) { - m.active_view = active_view_name; - } - Ok(m) -} - -fn parse_category_line(line: &str, cat_name: &str, m: &mut Model) { - if let Some(rest) = line.strip_prefix("- ") { - // Item line: "- ItemName" or "- ItemName [GroupName]" - let (item_name, group) = parse_bracketed(rest); - if let Some(c) = m.category_mut(cat_name) { - match group { - Some(g) => { - c.add_item_in_group(item_name, g); - if !c.groups.iter().any(|existing| existing.name == g) { - c.add_group(Group::new(g)); + Section::Category => { + let Some(cat) = categories.last_mut() else { continue }; + if let Some(rest) = trimmed.strip_prefix("- ") { + let (name, group) = parse_bracketed(rest); + cat.items.push((name.to_string(), group.map(str::to_string))); + } else if let Some(rest) = trimmed.strip_prefix("> ") { + let (group, parent) = parse_bracketed(rest); + if let Some(p) = parent { + cat.group_parents.push((group.to_string(), p.to_string())); } } - None => { c.add_item(item_name); } } - } - } else if let Some(rest) = line.strip_prefix("> ") { - // Group hierarchy line: "> GroupName [ParentName]" - let (group_name, parent) = parse_bracketed(rest); - if let Some(parent_name) = parent { - if let Some(c) = m.category_mut(cat_name) { - match c.groups.iter_mut().find(|g| g.name == group_name) { - Some(g) => g.parent = Some(parent_name.to_string()), - None => c.add_group(Group::new(group_name).with_parent(parent_name)), + Section::Formulas => { + if let Some(rest) = trimmed.strip_prefix("- ") { + let (raw, cat) = parse_bracketed(rest); + if let Some(c) = cat { + formulas.push((raw.to_string(), c.to_string())); + } } } - } - } -} - -fn parse_formula_line(line: &str, m: &mut Model) -> Result<()> { - if let Some(rest) = line.strip_prefix("- ") { - let (raw, cat) = parse_bracketed(rest); - if let Some(cat_name) = cat { - let formula = parse_formula(raw, cat_name) - .with_context(|| format!("Formula: {raw}"))?; - m.add_formula(formula); - } - } - Ok(()) -} - -fn parse_data_line(line: &str, m: &mut Model) { - // "Cat1=Item1, Cat2=Item2 = value" - let Some(sep) = line.find(" = ") else { return }; - let coords_str = &line[..sep]; - let value_str = line[sep + 3..].trim(); - let coords: Vec<(String, String)> = coords_str.split(", ") - .filter_map(|part| { - let (cat, item) = part.split_once('=')?; - Some((cat.trim().to_string(), item.trim().to_string())) - }) - .collect(); - if coords.is_empty() { return; } - let value = if let Some(inner) = value_str.strip_prefix('"').and_then(|s| s.strip_suffix('"')) { - CellValue::Text(inner.to_string()) - } else if let Ok(n) = value_str.parse::() { - CellValue::Number(n) - } else { - CellValue::Text(value_str.to_string()) - }; - m.set_cell(CellKey::new(coords), value); -} - -fn parse_view_line(line: &str, view_name: &str, m: &mut Model) -> Result<()> { - let view = m.views.get_mut(view_name) - .ok_or_else(|| anyhow::anyhow!("View not found: {}", view_name))?; - - if let Some(fmt) = line.strip_prefix("format: ") { - view.number_format = fmt.trim().to_string(); - } else if let Some(rest) = line.strip_prefix("hidden: ") { - if let Some((cat, item)) = rest.trim().split_once('/') { - view.hide_item(cat.trim(), item.trim()); - } - } else if let Some(rest) = line.strip_prefix("collapsed: ") { - if let Some((cat, group)) = rest.trim().split_once('/') { - view.toggle_group_collapse(cat.trim(), group.trim()); - } - } else if let Some(colon) = line.find(": ") { - let cat = line[..colon].trim(); - let rest = line[colon + 2..].trim(); - if let Some(sel_rest) = rest.strip_prefix("page") { - view.set_axis(cat, Axis::Page); - if let Some(sel) = sel_rest.strip_prefix(", ") { - view.set_page_selection(cat, sel.trim()); + Section::Data => { + let Some(sep) = trimmed.find(" = ") else { continue }; + let coords: Vec<(String, String)> = trimmed[..sep].split(", ") + .filter_map(|p| { let (c, i) = p.split_once('=')?; + Some((c.trim().to_string(), i.trim().to_string())) }) + .collect(); + if coords.is_empty() { continue; } + let vs = trimmed[sep + 3..].trim(); + let value = if let Some(s) = vs.strip_prefix('"').and_then(|s| s.strip_suffix('"')) { + CellValue::Text(s.to_string()) + } else if let Ok(n) = vs.parse::() { + CellValue::Number(n) + } else { + CellValue::Text(vs.to_string()) + }; + data.push((CellKey::new(coords), value)); } - } else { - let axis = match rest { - "row" => Axis::Row, - "column" => Axis::Column, - _ => return Ok(()), - }; - view.set_axis(cat, axis); + Section::View => { + let Some(view) = views.last_mut() else { continue }; + if let Some(fmt) = trimmed.strip_prefix("format: ") { + view.format = fmt.trim().to_string(); + } else if let Some(rest) = trimmed.strip_prefix("hidden: ") { + if let Some((c, i)) = rest.trim().split_once('/') { + view.hidden.push((c.trim().to_string(), i.trim().to_string())); + } + } else if let Some(rest) = trimmed.strip_prefix("collapsed: ") { + if let Some((c, g)) = rest.trim().split_once('/') { + view.collapsed.push((c.trim().to_string(), g.trim().to_string())); + } + } else if let Some(colon) = trimmed.find(": ") { + let cat = trimmed[..colon].trim(); + let rest = trimmed[colon + 2..].trim(); + if let Some(sel_rest) = rest.strip_prefix("page") { + view.axes.push((cat.to_string(), Axis::Page)); + if let Some(sel) = sel_rest.strip_prefix(", ") { + view.page_selections.push((cat.to_string(), sel.trim().to_string())); + } + } else { + let axis = match rest { "row" => Axis::Row, "column" => Axis::Column, + _ => continue }; + view.axes.push((cat.to_string(), axis)); + } + } + } + Section::None => {} } } - Ok(()) + + // ── Pass 2: build ───────────────────────────────────────────────────────── + + let name = model_name.ok_or_else(|| anyhow::anyhow!("Missing model title (# Name)"))?; + let mut m = Model::new(&name); + + // Categories first — registers them with all existing views via on_category_added + for pc in &categories { + m.add_category(&pc.name)?; + let cat = m.category_mut(&pc.name).unwrap(); + for (item_name, group) in &pc.items { + match group { + Some(g) => { + cat.add_item_in_group(item_name, g); + if !cat.groups.iter().any(|e| &e.name == g) { + cat.add_group(Group::new(g)); + } + } + None => { cat.add_item(item_name); } + } + } + for (group_name, parent) in &pc.group_parents { + match cat.groups.iter_mut().find(|g| &g.name == group_name) { + Some(g) => g.parent = Some(parent.clone()), + None => cat.add_group(Group::new(group_name).with_parent(parent)), + } + } + } + + // Views — all categories are now registered, so set_axis works correctly + let mut active_view = String::new(); + for pv in &views { + if pv.is_active { active_view = pv.name.clone(); } + if !m.views.contains_key(&pv.name) { m.create_view(&pv.name); } + let view = m.views.get_mut(&pv.name).unwrap(); + for (cat, axis) in &pv.axes { view.set_axis(cat, *axis); } + for (cat, sel) in &pv.page_selections { view.set_page_selection(cat, sel); } + if !pv.format.is_empty() { view.number_format = pv.format.clone(); } + for (cat, item) in &pv.hidden { view.hide_item(cat, item); } + for (cat, grp) in &pv.collapsed { view.toggle_group_collapse(cat, grp); } + } + if !active_view.is_empty() && m.views.contains_key(&active_view) { + m.active_view = active_view; + } + + // Formulas and data can go in any order relative to each other + for (raw, cat_name) in &formulas { + m.add_formula(parse_formula(raw, cat_name) + .with_context(|| format!("Formula: {raw}"))?); + } + for (key, value) in data { + m.set_cell(key, value); + } + + Ok(m) } /// Split `"Name [Bracket]"` → `("Name", Some("Bracket"))` or `("Name", None)`. @@ -585,6 +608,65 @@ mod tests { } } + #[test] + fn parse_md_order_independent_view_before_categories() { + // A hand-edited file with the view section before the category sections. + // The parser must still produce correct axis assignments. + let text = "# Test\n\ + ## View: Default (active)\n\ + Type: row\n\ + Month: column\n\ + ## Category: Type\n\ + - Food\n\ + ## Category: Month\n\ + - Jan\n"; + let m = parse_md(text).unwrap(); + assert_eq!(m.active_view().axis_of("Type"), Axis::Row); + assert_eq!(m.active_view().axis_of("Month"), Axis::Column); + } + + #[test] + fn parse_md_order_independent_new_view_before_categories() { + // A non-Default view with swapped axes, declared before categories exist. + let text = "# Test\n\ + ## View: Transposed (active)\n\ + Type: column\n\ + Month: row\n\ + ## View: Default\n\ + Type: row\n\ + Month: column\n\ + ## Category: Type\n\ + - Food\n\ + ## Category: Month\n\ + - Jan\n"; + let m = parse_md(text).unwrap(); + let transposed = m.views.get("Transposed").unwrap(); + assert_eq!(transposed.axis_of("Type"), Axis::Column); + assert_eq!(transposed.axis_of("Month"), Axis::Row); + let default = m.views.get("Default").unwrap(); + assert_eq!(default.axis_of("Type"), Axis::Row); + assert_eq!(default.axis_of("Month"), Axis::Column); + } + + #[test] + fn parse_md_order_independent_data_before_categories() { + let text = "# Test\n\ + ## Data\n\ + Month=Jan, Type=Food = 42\n\ + ## Category: Type\n\ + - Food\n\ + ## Category: Month\n\ + - Jan\n\ + ## View: Default (active)\n\ + Type: row\n\ + Month: column\n"; + let m = parse_md(text).unwrap(); + assert_eq!( + m.get_cell(&coord(&[("Month", "Jan"), ("Type", "Food")])), + Some(&CellValue::Number(42.0)) + ); + } + #[test] fn load_dispatcher_detects_legacy_json_by_brace() { // The load() function routes to JSON deserializer when text starts with '{'