From e65d87fda999ced0a3d8afe9be260427d7f234dc Mon Sep 17 00:00:00 2001 From: "Claude (agent-lpzu)" Date: Thu, 24 Sep 2026 20:43:31 -0700 Subject: [PATCH 1/6] feat(import): import a folder of CSVs as one sparse model (improvise-lpzu, improvise-8jk6.2) A folder argument expands to its *.csv documents through Storage::list, and a record with no value for a category keeps its cell without that coordinate instead of being skipped. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-io/src/import/csv_parser.rs | 145 ++++++++- crates/improvise-io/src/import/wizard.rs | 296 +++++++++++-------- src/main.rs | 19 +- tests/headless_cli.rs | 84 ++++++ 4 files changed, 412 insertions(+), 132 deletions(-) diff --git a/crates/improvise-io/src/import/csv_parser.rs b/crates/improvise-io/src/import/csv_parser.rs index 0075740b..18d353e5 100644 --- a/crates/improvise-io/src/import/csv_parser.rs +++ b/crates/improvise-io/src/import/csv_parser.rs @@ -1,10 +1,12 @@ use std::collections::HashSet; -use std::path::Path; +use std::path::{Path, PathBuf}; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, ensure}; use csv::ReaderBuilder; use serde_json::Value; +use crate::storage::{PhysicalFs, Storage}; + pub fn csv_path_p(path: &Path) -> bool { path.extension() .is_some_and(|ext| ext.eq_ignore_ascii_case("csv")) @@ -145,7 +147,21 @@ fn disambiguate_headers(raw: Vec) -> (Vec, Vec) { /// Parse multiple CSV files and merge into a single JSON array. /// Each record gets a "File" field set to the filename stem (e.g., "sales" from "sales.csv"). pub fn merge_csvs(paths: &[impl AsRef]) -> Result> { + let (records, warnings) = merge_csvs_from(&PhysicalFs, paths)?; + for warning in &warnings { + eprintln!("warning: {warning}"); + } + Ok(records) +} + +/// As [`merge_csvs`] over any [`Storage`], returning each file's warnings +/// prefixed with its path. Files may have different headers (improvise-lpzu). +pub fn merge_csvs_from( + fs: &dyn Storage, + paths: &[impl AsRef], +) -> Result<(Vec, Vec)> { let mut all_records = Vec::new(); + let mut all_warnings = Vec::new(); for path in paths { let path = path.as_ref(); let stem = path @@ -153,7 +169,10 @@ pub fn merge_csvs(paths: &[impl AsRef]) -> Result> { .and_then(|s| s.to_str()) .unwrap_or("unknown") .to_string(); - let records = parse_csv(path)?; + let bytes = fs.read(path)?; + let (records, warnings) = parse_csv_bytes_with_warnings(&bytes) + .with_context(|| format!("Failed to parse CSV file: {}", path.display()))?; + all_warnings.extend(warnings.iter().map(|w| format!("{}: {w}", path.display()))); for mut record in records { if let Value::Object(ref mut map) = record { map.insert("File".to_string(), Value::String(stem.clone())); @@ -161,7 +180,40 @@ pub fn merge_csvs(paths: &[impl AsRef]) -> Result> { all_records.push(record); } } - Ok(all_records) + Ok((all_records, all_warnings)) +} + +/// One `improvise import` argument. Whether a path names a folder is the +/// host's question, so the caller classifies and [`expand_import_paths`] lists. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ImportPath { + File(PathBuf), + Folder(PathBuf), +} + +/// The files an import reads: each argument in turn, a folder standing for its +/// `*.csv` documents in name order. See `context/code/formula-and-io.md`. +pub fn expand_import_paths(fs: &dyn Storage, args: &[ImportPath]) -> Result> { + let mut paths = Vec::new(); + for arg in args { + match arg { + ImportPath::File(path) => paths.push(path.clone()), + ImportPath::Folder(dir) => paths.extend(csv_documents_in(fs, dir)?), + } + } + Ok(paths) +} + +/// A folder's `*.csv` documents, sorted by `Storage::list`'s contract. A folder +/// with none is refused by name rather than imported as nothing. +fn csv_documents_in(fs: &dyn Storage, dir: &Path) -> Result> { + let csvs: Vec = fs + .list(dir)? + .into_iter() + .filter(|p| csv_path_p(p)) + .collect(); + ensure!(!csvs.is_empty(), "no CSV files in folder {}", dir.display()); + Ok(csvs) } fn parse_csv_field(field: &str) -> Value { @@ -302,6 +354,91 @@ mod tests { assert_eq!(records[0]["Name"], Value::String("A".to_string())); } + // ── Folder arguments (improvise-lpzu) ───────────────────────────── + + use crate::storage::MemoryFs; + + fn folder(files: &[(&str, &str)]) -> MemoryFs { + let fs = MemoryFs::new(); + for (path, body) in files { + fs.write(Path::new(path), body.as_bytes()).unwrap(); + } + fs + } + + fn names(paths: &[PathBuf]) -> Vec { + paths.iter().map(|p| p.display().to_string()).collect() + } + + /// Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_folder_expands_to_its_csv_files_in_name_order_and_skips_the_rest() { + let fs = folder(&[ + ("maat/coupling.csv", "entity\n"), + ("maat/notes.txt", "not data"), + ("maat/age.CSV", "entity\n"), + ("maat/out.improv", "v2025-04-09\n"), + ("maat/abs-churn.csv", "date\n"), + ("elsewhere/other.csv", "x\n"), + ]); + let paths = expand_import_paths(&fs, &[ImportPath::Folder("maat".into())]).unwrap(); + assert_eq!( + names(&paths), + ["maat/abs-churn.csv", "maat/age.CSV", "maat/coupling.csv"] + ); + } + + /// Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_folder_with_no_csv_files_is_refused_by_name() { + let only_text = folder(&[("maat/notes.txt", "x"), ("maat/out.improv", "y")]); + let empty = MemoryFs::new(); + for fs in [&only_text, &empty] { + let err = expand_import_paths(fs, &[ImportPath::Folder("maat".into())]) + .expect_err("a folder with nothing to import is refused"); + assert_eq!(err.to_string(), "no CSV files in folder maat"); + } + } + + /// Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn files_and_folders_expand_in_argument_order() { + let fs = folder(&[ + ("b/two.csv", "x\n"), + ("b/one.csv", "x\n"), + ("a.csv", "x\n"), + ("z.csv", "x\n"), + ]); + let args = [ + ImportPath::File("z.csv".into()), + ImportPath::Folder("b".into()), + ImportPath::File("a.csv".into()), + ]; + let paths = expand_import_paths(&fs, &args).unwrap(); + assert_eq!(names(&paths), ["z.csv", "b/one.csv", "b/two.csv", "a.csv"]); + } + + /// Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn files_with_different_headers_merge_without_inventing_the_missing_columns() { + let fs = folder(&[ + ( + "m/coupling.csv", + "entity,coupled,degree\nsrc/a.rs,src/b.rs,80\n", + ), + ("m/age.csv", "entity,age-months\nsrc/a.rs,2\n"), + ]); + let (records, warnings) = merge_csvs_from(&fs, &["m/coupling.csv", "m/age.csv"]).unwrap(); + assert!(warnings.is_empty(), "{warnings:?}"); + assert_eq!( + records, + [ + serde_json::json!({"entity": "src/a.rs", "coupled": "src/b.rs", "degree": 80, "File": "coupling"}), + serde_json::json!({"entity": "src/a.rs", "age-months": 2, "File": "age"}), + ] + ); + } + // ── RFC 4180 edge cases ─────────────────────────────────────────── #[test] diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index 28efbdff..87464ca1 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -257,9 +257,6 @@ impl ImportPipeline { wb.add_item("_Measure", &m.field)?; } - // Per-category tally of records dropped because that category had no - // value; a record is charged to the first category it is missing. - let mut skipped_per_category = vec![0usize; categories.len()]; let mut skipped_non_objects = 0usize; // Per-extraction tally of values the detected format could not parse. A // derived coordinate that is merely absent is improvise-yss5. @@ -277,12 +274,6 @@ impl ImportPipeline { for (at, placement) in placements.into_iter().enumerate() { match placement { Placement::NotObject => skipped_non_objects += 1, - Placement::Missing { cat_idx, partial } => { - for (cat, item) in &partial { - wb.add_item(cat, item)?; - } - skipped_per_category[cat_idx] += 1; - } Placement::Placed { mut coords, measures, @@ -339,12 +330,9 @@ impl ImportPipeline { } } - let mut warnings = skip_warnings( - self.records.len(), - categories, - &skipped_per_category, - skipped_non_objects, - ); + let mut warnings: Vec = skip_warnings(self.records.len(), skipped_non_objects) + .into_iter() + .collect(); warnings.extend(unparsed_date_warnings(date_extractions, &unparsed_dates)); warnings.extend(collision_warning(&summed)); warnings.extend(occurrence.map(|name| separated_note(name, &collisions))); @@ -371,11 +359,6 @@ enum Placement { measures: Vec<(String, f64)>, unparsed: Vec, }, - /// No value for category `cat_idx`; `partial` is what preceded it. - Missing { - cat_idx: usize, - partial: Vec<(String, String)>, - }, NotObject, } @@ -385,17 +368,15 @@ impl Plan<'_> { return Placement::NotObject; }; let mut coords: Vec<(String, String)> = vec![]; - // Charged only once this record survives the walk: a row dropped - // for a LATER missing category never lands (context/test-evidence). let mut unparsed: Vec = vec![]; - for (cat_idx, cat_proposal) in self.categories.iter().enumerate() { - let Some(v) = map.get(&cat_proposal.field).and_then(scalar_name) else { - return Placement::Missing { - cat_idx, - partial: coords, - }; - }; + // No value is no coordinate: records are sparse (improvise-8jk6.2). + let named = |field: &str| map.get(field).and_then(scalar_name); + for (cat_proposal, v) in self + .categories + .iter() + .filter_map(|p| Some((p, named(&p.field)?))) + { coords.push((cat_proposal.field.clone(), v.clone())); // Extract date components from this field's value @@ -410,13 +391,13 @@ impl Plan<'_> { } } - // Attach label values as coords (missing labels become ""). - for lab in &self.labels { - let val = map - .get(&lab.field) - .and_then(scalar_name) - .unwrap_or_default(); - coords.push((lab.field.clone(), val)); + // Labels likewise: a blank label is no coordinate, never an item "". + for (lab, v) in self + .labels + .iter() + .filter_map(|lab| Some((lab, named(&lab.field)?))) + { + coords.push((lab.field.clone(), v)); } let measures = self @@ -538,37 +519,12 @@ fn unparsed_date_warnings( .collect() } -/// Human-readable warnings for records that could not be placed in the grid. -/// Empty when every record was imported. -fn skip_warnings( - total: usize, - categories: &[&FieldProposal], - skipped_per_category: &[usize], - skipped_non_objects: usize, -) -> Vec { - let mut warnings = vec![]; - - let missing_total: usize = skipped_per_category.iter().sum(); - if missing_total > 0 { - let details: Vec = categories - .iter() - .zip(skipped_per_category) - .filter(|(_, n)| **n > 0) - .map(|(p, n)| format!("{} ({n})", p.field)) - .collect(); - warnings.push(format!( - "Skipped {missing_total} of {total} records missing category values: {}", - details.join(", ") - )); - } - - if skipped_non_objects > 0 { - warnings.push(format!( - "Skipped {skipped_non_objects} of {total} records that were not JSON objects" - )); - } - - warnings +/// The warning for records that could not be placed in the grid. A record +/// missing a category value is placed, so only a non-object is skipped. +fn skip_warnings(total: usize, skipped_non_objects: usize) -> Option { + (skipped_non_objects > 0).then(|| { + format!("Skipped {skipped_non_objects} of {total} records that were not JSON objects") + }) } // ── Wizard (UI state wrapped around the pipeline) ───────────────────────────── @@ -1345,10 +1301,9 @@ mod tests { ); } + /// Evidence: `context/test-evidence/formula-and-io.md`. #[test] - fn a_row_dropped_for_a_missing_category_is_not_charged_to_the_date_tally() { - // `Date` is walked before `Region`, so a bad date is seen first and the - // record is only afterwards dropped for the missing `Region`. + fn a_row_kept_without_a_category_is_charged_to_the_date_tally() { let mut p = extracting_month_and_year(json!([ {"Date": "01/15/2025", "Region": "East", "Amount": 100.0}, {"Date": "2025-02-05", "Amount": 200.0}, @@ -1367,23 +1322,23 @@ mod tests { }) .map(|p| p.field.as_str()) .collect(); - assert_eq!( - categories, - vec!["Date", "Region"], - "the fixture only discriminates while Date is walked FIRST" - ); + assert_eq!(categories, vec!["Date", "Region"]); - let (_, warnings) = p.build_model_with_warnings().unwrap(); + let (wb, warnings) = p.build_model_with_warnings().unwrap(); assert!( - warnings.iter().all(|w| !w.contains("Date_Month")), - "the row never landed, so no derived column lost it: {warnings:?}" + stored(&wb).contains(&"Date=2025-02-05, _Measure=Amount = 200".to_string()), + "the row lands without Region or a derived part: {:?}", + stored(&wb) ); - assert_eq!( - warnings.len(), - 1, - "only the skipped-record warning: {warnings:?}" + assert_eq!(warnings.len(), 2, "{warnings:?}"); + assert!( + warnings[0].contains("'Date_Month' missing from 1 row(s)"), + "{warnings:?}" + ); + assert!( + warnings[1].contains("'Date_Year' missing from 1 row(s)"), + "{warnings:?}" ); - assert!(warnings[0].contains("Region"), "{}", warnings[0]); } #[test] @@ -2005,14 +1960,15 @@ mod tests { "null must not become an item: {items:?}" ); assert_eq!( - warnings.len(), - 1, - "the dropped record is reported: {warnings:?}" + stored(&wb), + [ + "_Measure=revenue, region=East = 100", + "_Measure=revenue = 200" + ] ); assert!( - warnings[0].contains("region"), - "warning names the field: {}", - warnings[0] + warnings.is_empty(), + "the record is kept, not skipped: {warnings:?}" ); } @@ -2060,9 +2016,15 @@ mod tests { .collect(); assert!(!items.contains(&""), "no empty-named item: {items:?}"); assert_eq!( - warnings.len(), - 1, - "the dropped record is reported: {warnings:?}" + stored(&wb), + [ + "_Measure=revenue, region=East = 100", + "_Measure=revenue = 200" + ] + ); + assert!( + warnings.is_empty(), + "the record is kept, not skipped: {warnings:?}" ); } @@ -2090,55 +2052,135 @@ mod tests { assert!(warnings.is_empty(), "nothing dropped: {warnings:?}"); } + /// improvise-8jk6.2: a record without the field keeps its cell. It was + /// skipped until records became sparse. #[test] - fn build_model_record_with_missing_category_value_skipped() { - // If a record is missing a category field, it should be skipped + fn build_model_record_with_missing_category_field_is_kept() { let raw = json!([ {"region": "East", "revenue": 100.0}, {"revenue": 200.0}, // missing "region" ]); - let p = ImportPipeline::new(raw); - let wb = p.build_model().unwrap(); - // Only one cell should exist (the East record) - use crate::model::cell::CellKey; - let k = CellKey::new(vec![ - ("_Measure".to_string(), "revenue".to_string()), - ("region".to_string(), "East".to_string()), - ]); - assert!(wb.model.get_cell(&k).is_some()); + let wb = ImportPipeline::new(raw).build_model().unwrap(); + assert_eq!( + stored(&wb), + [ + "_Measure=revenue, region=East = 100", + "_Measure=revenue = 200" + ] + ); } - /// improvise-5j9: skipping a record for a missing category value was - /// silent. The warning must name the count, the total and the field(s). + /// improvise-8jk6.2: a blank category value used to drop the whole record. + /// Evidence: `context/test-evidence/formula-and-io.md`. #[test] - fn build_model_warns_about_records_skipped_for_missing_category() { + fn a_record_with_no_value_for_a_category_is_kept_without_that_coordinate() { let raw = json!([ - {"region": "East", "revenue": 100.0}, - {"revenue": 200.0}, // missing "region" - {"revenue": 300.0}, // missing "region" + {"Payee": "Acme", "Settlement Date": "2026-01-05", "Amount": 10.0}, + {"Payee": "Bolt", "Settlement Date": null, "Amount": 20.0}, + {"Payee": "Acme", "Settlement Date": "", "Amount": 40.0}, ]); - let p = ImportPipeline::new(raw); - let (_, warnings) = p.build_model_with_warnings().unwrap(); + let mut p = ImportPipeline::new(raw); + force_category(&mut p, "Settlement Date"); + let (wb, warnings) = p.build_model_with_warnings().unwrap(); + assert_eq!( - warnings.len(), - 1, - "one skipped-record warning: {warnings:?}" + stored(&wb), + [ + "Payee=Acme, Settlement Date=2026-01-05, _Measure=Amount = 10", + "Payee=Bolt, _Measure=Amount = 20", + "Payee=Acme, _Measure=Amount = 40", + ] ); - assert!( - warnings[0].contains('2'), - "warning counts skipped rows: {}", - warnings[0] + let items = &wb.model.category("Settlement Date").unwrap().items; + assert_eq!(items.keys().collect::>(), ["2026-01-05"]); + assert!(warnings.is_empty(), "{warnings:?}"); + } + + /// code-maat's coupling and age reports, as a folder import merges them. + fn code_maat_records() -> Value { + use crate::import::csv_parser::merge_csvs_from; + use crate::storage::{MemoryFs, Storage}; + let fs = MemoryFs::new(); + let files = [ + ( + "maat/age.csv", + "entity,age-months\nsrc/a.rs,2\nsrc/c.rs,7\n", + ), + ( + "maat/coupling.csv", + "entity,coupled,degree,average-revs\nsrc/a.rs,src/b.rs,80,5\nsrc/b.rs,src/c.rs,40,3\n", + ), + ]; + for (path, body) in files { + fs.write(std::path::Path::new(path), body.as_bytes()) + .unwrap(); + } + let (records, warnings) = merge_csvs_from(&fs, &files.map(|(p, _)| p)).unwrap(); + assert!(warnings.is_empty(), "{warnings:?}"); + Value::Array(records) + } + + /// improvise-lpzu: a column a file lacks is absent from its records, never + /// 0, "" or a skip. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn files_with_different_headers_import_as_one_sparse_model() { + let p = ImportPipeline::new(code_maat_records()); + let kinds: Vec<(&str, FieldKind)> = p + .proposals + .iter() + .map(|f| (f.field.as_str(), f.kind.clone())) + .collect(); + for (field, kind) in [ + ("entity", FieldKind::Category), + ("coupled", FieldKind::Category), + ("File", FieldKind::Category), + ("age-months", FieldKind::Measure), + ("degree", FieldKind::Measure), + ("average-revs", FieldKind::Measure), + ] { + assert!(kinds.contains(&(field, kind.clone())), "{field}: {kinds:?}"); + } + + let (wb, warnings) = p.build_model_with_warnings().unwrap(); + let mut cells = stored(&wb); + cells.sort(); + assert_eq!( + cells, + [ + "File=age, _Measure=age-months, entity=src/a.rs = 2", + "File=age, _Measure=age-months, entity=src/c.rs = 7", + "File=coupling, _Measure=average-revs, coupled=src/b.rs, entity=src/a.rs = 5", + "File=coupling, _Measure=average-revs, coupled=src/c.rs, entity=src/b.rs = 3", + "File=coupling, _Measure=degree, coupled=src/b.rs, entity=src/a.rs = 80", + "File=coupling, _Measure=degree, coupled=src/c.rs, entity=src/b.rs = 40", + ] ); - assert!( - warnings[0].contains('3'), - "warning names the total: {}", - warnings[0] - ); - assert!( - warnings[0].contains("region"), - "warning names the missing field: {}", - warnings[0] + for cat in wb.model.regular_category_names() { + let items = &wb.model.category(cat).unwrap().items; + assert!(!items.contains_key(""), "{cat} has an empty item"); + } + assert!(warnings.is_empty(), "{warnings:?}"); + } + + /// improvise-lpzu: the label half of the rule — absent, not an item named + /// "". Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_label_column_a_record_lacks_is_no_coordinate_not_an_empty_item() { + let mut p = ImportPipeline::new(code_maat_records()); + p.proposals + .iter_mut() + .find(|f| f.field == "coupled") + .unwrap() + .kind = FieldKind::Label; + let (wb, _) = p.build_model_with_warnings().unwrap(); + let mut cells = stored(&wb); + cells.sort(); + assert_eq!( + cells[0], + "File=age, _Measure=age-months, entity=src/a.rs = 2" ); + let items = &wb.model.category("coupled").unwrap().items; + assert!(!items.contains_key(""), "{:?}", items.keys()); } #[test] diff --git a/src/main.rs b/src/main.rs index f2144274..04dc835d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -330,7 +330,24 @@ fn run_wizard_import( // ── Import data loading ────────────────────────────────────────────────────── -fn get_import_data(paths: &[PathBuf]) -> Option { +fn get_import_data(args: &[PathBuf]) -> Option { + use improvise::import::csv_parser::{ImportPath, expand_import_paths}; + // Whether an argument is a folder is the one filesystem question asked here. + let classified: Vec = args + .iter() + .map(|p| match p.is_dir() { + true => ImportPath::Folder(p.clone()), + false => ImportPath::File(p.clone()), + }) + .collect(); + let paths = match expand_import_paths(&improvise_io::storage::PhysicalFs, &classified) { + Ok(paths) => paths, + Err(e) => { + eprintln!("{e:#}"); + return None; + } + }; + let paths = paths.as_slice(); let all_csv = paths.iter().all(|p| csv_path_p(p)); if paths.len() > 1 { diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index 5af8a6c2..4d4bdeec 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -612,3 +612,87 @@ fn a_headless_import_keeps_duplicates_separate_when_told_to() { assert!(doc.contains("= 10") && doc.contains("= 20"), "{doc}"); assert!(!doc.contains("= 30"), "{doc}"); } + +/// code-maat's reports: three schemas, a stray note, and room for the output. +fn code_maat_folder(scratch: &Scratch) -> PathBuf { + let dir = scratch.path("maat"); + std::fs::create_dir_all(&dir).unwrap(); + for (name, body) in [ + ( + "coupling.csv", + "entity,coupled,degree,average-revs\nsrc/a.rs,src/b.rs,80,5\n", + ), + ("age.csv", "entity,age-months\nsrc/a.rs,2\nsrc/b.rs,9\n"), + ( + "abs-churn.csv", + "date,added,deleted,commits\n2026-01-02,10,3,2\n", + ), + ("README.txt", "not a report\n"), + ] { + std::fs::write(dir.join(name), body).unwrap(); + } + dir +} + +/// improvise-lpzu: a folder argument is its `*.csv` files, merged sparsely and +/// in name order. Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_headless_import_of_a_folder_merges_its_csv_files_and_repeats_byte_for_byte() { + let scratch = Scratch::new("folder_import"); + let dir = code_maat_folder(&scratch); + let out_path = dir.join("maat.improv"); + + let first = run_import(&dir, &out_path, &[]); + assert!(first.status.success(), "{}", stderr_of(&first)); + let doc = std::fs::read_to_string(&out_path).unwrap(); + for line in [ + "File=coupling, _Measure=degree, coupled=|src/b.rs|, entity=|src/a.rs| = 80", + "File=age, _Measure=age-months, entity=|src/b.rs| = 9", + "File=abs-churn, _Measure=added, date=|2026-01-02| = 10", + ] { + assert!(doc.lines().any(|l| l == line), "{line} missing from\n{doc}"); + } + assert!(!doc.contains("README"), "{doc}"); + + // The output now sits in the folder; a second run must not read it. + let second = run_import(&dir, &out_path, &[]); + assert!(second.status.success(), "{}", stderr_of(&second)); + assert_eq!(std::fs::read_to_string(&out_path).unwrap(), doc); +} + +/// improvise-lpzu: a file and a folder together are one merge, and a folder +/// with nothing to import is named. Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_headless_import_takes_files_and_folders_together_and_refuses_an_empty_folder() { + let scratch = Scratch::new("folder_mixed"); + let dir = code_maat_folder(&scratch); + let extra = scratch.path("extra.csv"); + std::fs::write(&extra, "entity,age-months\nsrc/z.rs,4\n").unwrap(); + let out_path = scratch.path("out.improv"); + + let out = Command::new(bin()) + .args(["import", "--no-wizard", "-o"]) + .arg(&out_path) + .arg(&extra) + .arg(&dir) + .output() + .unwrap(); + assert!(out.status.success(), "{}", stderr_of(&out)); + let doc = std::fs::read_to_string(&out_path).unwrap(); + assert!( + doc.contains("File=extra, _Measure=age-months, entity=|src/z.rs| = 4"), + "{doc}" + ); + assert!(doc.contains("File=coupling"), "{doc}"); + + let empty = scratch.path("empty"); + std::fs::create_dir_all(&empty).unwrap(); + let refused = run_import(&empty, &scratch.path("never.improv"), &[]); + assert!(!refused.status.success()); + let err = stderr_of(&refused); + assert!( + err.contains(&format!("no CSV files in folder {}", empty.display())), + "{err}" + ); + assert!(!scratch.path("never.improv").exists()); +} From bb505f8faf164674530f939c331dd3ff7aa0f5d7 Mon Sep 17 00:00:00 2001 From: "Claude (agent-lpzu)" Date: Thu, 24 Sep 2026 20:48:16 -0700 Subject: [PATCH 2/6] feat(import): give each merged file a view filtered to it (improvise-lpzu) A CSV merge now records each file's columns, and the build adds one view per file: File on Filter at that file's item, its own categories seated as a fresh import seats them, the rest on None, and other files' measures hidden. The merged Default view stays the initial view. The headless and wizard paths both carry the sources. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/app.rs | 7 +- crates/improvise-command/src/ui/effect.rs | 1 + crates/improvise-io/src/import/csv_parser.rs | 101 +++++++-- crates/improvise-io/src/import/wizard.rs | 215 +++++++++++++++++-- src/draw.rs | 6 +- src/main.rs | 35 +-- tests/headless_cli.rs | 12 ++ 7 files changed, 322 insertions(+), 55 deletions(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 89406d91..1b3f84c0 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -1224,7 +1224,12 @@ impl App { } pub fn start_import_wizard(&mut self, json: serde_json::Value) { - self.wizard = Some(ImportWizard::new(json)); + self.start_import(crate::import::wizard::ImportPipeline::new(json)); + } + + /// Open the wizard over a pipeline the host built, sources and all. + pub fn start_import(&mut self, pipeline: crate::import::wizard::ImportPipeline) { + self.wizard = Some(ImportWizard::from_pipeline(pipeline)); self.view_state.mode = AppMode::ImportWizard; } diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 0251d4a0..84173f4f 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1315,6 +1315,7 @@ impl Effect for ImportJsonHeadless { formulas: vec![], // Headless: nobody to ask, so a collision refuses (improvise-cgqt). on_duplicate: DuplicatePolicy::Refuse, + sources: vec![], }; match pipeline.build_model_with_warnings() { diff --git a/crates/improvise-io/src/import/csv_parser.rs b/crates/improvise-io/src/import/csv_parser.rs index 18d353e5..6f2120de 100644 --- a/crates/improvise-io/src/import/csv_parser.rs +++ b/crates/improvise-io/src/import/csv_parser.rs @@ -33,6 +33,12 @@ pub fn parse_csv_with_warnings(path: &Path) -> Result<(Vec, Vec)> /// As [`parse_csv_with_warnings`] but over bytes, so import can come from any /// [`Storage`](crate::storage::Storage) backend, not only a file (improvise-fuj). pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(Vec, Vec)> { + let (_, records, warnings) = parse_csv_bytes(bytes)?; + Ok((records, warnings)) +} + +/// The header (after renaming), the records and the warnings. +fn parse_csv_bytes(bytes: &[u8]) -> Result<(Vec, Vec, Vec)> { let mut reader = ReaderBuilder::new() .has_headers(true) .flexible(true) @@ -112,7 +118,7 @@ pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(Vec, Vec) -> (Vec, Vec) { /// Parse multiple CSV files and merge into a single JSON array. /// Each record gets a "File" field set to the filename stem (e.g., "sales" from "sales.csv"). pub fn merge_csvs(paths: &[impl AsRef]) -> Result> { - let (records, warnings) = merge_csvs_from(&PhysicalFs, paths)?; - for warning in &warnings { + let merged = merge_csvs_from(&PhysicalFs, paths)?; + for warning in &merged.warnings { eprintln!("warning: {warning}"); } - Ok(records) + Ok(merged.records) } -/// As [`merge_csvs`] over any [`Storage`], returning each file's warnings -/// prefixed with its path. Files may have different headers (improvise-lpzu). -pub fn merge_csvs_from( - fs: &dyn Storage, - paths: &[impl AsRef], -) -> Result<(Vec, Vec)> { - let mut all_records = Vec::new(); - let mut all_warnings = Vec::new(); +/// The category every merged record names its source file by. +pub const FILE_CATEGORY: &str = "File"; + +/// One merged file: its `File` item and the columns its header named. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SourceFile { + pub item: String, + pub columns: Vec, +} + +/// Several CSV files as one record set, with each file's own columns kept. +#[derive(Debug, Clone, PartialEq)] +pub struct Merged { + pub records: Vec, + /// One per distinct `File` item, in the order the files were read. + pub sources: Vec, + /// Each file's parse warnings, prefixed with its path. + pub warnings: Vec, +} + +/// As [`merge_csvs`] over any [`Storage`]. Files may have different headers, +/// and two files with one stem share its item and its columns (improvise-lpzu). +pub fn merge_csvs_from(fs: &dyn Storage, paths: &[impl AsRef]) -> Result { + let mut merged = Merged { + records: Vec::new(), + sources: Vec::new(), + warnings: Vec::new(), + }; for path in paths { let path = path.as_ref(); let stem = path @@ -170,17 +196,41 @@ pub fn merge_csvs_from( .unwrap_or("unknown") .to_string(); let bytes = fs.read(path)?; - let (records, warnings) = parse_csv_bytes_with_warnings(&bytes) + let (headers, records, warnings) = parse_csv_bytes(&bytes) .with_context(|| format!("Failed to parse CSV file: {}", path.display()))?; - all_warnings.extend(warnings.iter().map(|w| format!("{}: {w}", path.display()))); + let at = merged.source_for(&stem); + let columns = &mut merged.sources[at].columns; + for header in headers { + if header != FILE_CATEGORY && !columns.contains(&header) { + columns.push(header); + } + } + merged + .warnings + .extend(warnings.iter().map(|w| format!("{}: {w}", path.display()))); for mut record in records { if let Value::Object(ref mut map) = record { - map.insert("File".to_string(), Value::String(stem.clone())); + map.insert(FILE_CATEGORY.to_string(), Value::String(stem.clone())); + } + merged.records.push(record); + } + } + Ok(merged) +} + +impl Merged { + fn source_for(&mut self, item: &str) -> usize { + match self.sources.iter().position(|s| s.item == item) { + Some(at) => at, + None => { + self.sources.push(SourceFile { + item: item.to_string(), + columns: vec![], + }); + self.sources.len() - 1 } - all_records.push(record); } } - Ok((all_records, all_warnings)) } /// One `improvise import` argument. Whether a path names a folder is the @@ -428,10 +478,21 @@ mod tests { ), ("m/age.csv", "entity,age-months\nsrc/a.rs,2\n"), ]); - let (records, warnings) = merge_csvs_from(&fs, &["m/coupling.csv", "m/age.csv"]).unwrap(); - assert!(warnings.is_empty(), "{warnings:?}"); + let merged = merge_csvs_from(&fs, &["m/coupling.csv", "m/age.csv"]).unwrap(); + assert!(merged.warnings.is_empty(), "{:?}", merged.warnings); + let source = |item: &str, columns: &[&str]| SourceFile { + item: item.to_string(), + columns: columns.iter().map(|c| c.to_string()).collect(), + }; assert_eq!( - records, + merged.sources, + [ + source("coupling", &["entity", "coupled", "degree"]), + source("age", &["entity", "age-months"]), + ] + ); + assert_eq!( + merged.records, [ serde_json::json!({"entity": "src/a.rs", "coupled": "src/b.rs", "degree": 80, "File": "coupling"}), serde_json::json!({"entity": "src/a.rs", "age-months": 2, "File": "age"}), diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index 87464ca1..d3283849 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -7,10 +7,11 @@ use super::analyzer::{ DateComponent, FieldKind, FieldProposal, analyze_records, extract_array_at_path, extract_date_component, find_array_paths, }; +use super::csv_parser::{FILE_CATEGORY, SourceFile}; use crate::formula::parse_formula; use crate::model::MAX_CATEGORIES; use crate::model::cell::{CellKey, CellValue}; -use crate::view::Axis; +use crate::view::{Axis, VIRTUAL_VIEW_NAMES, View}; use crate::workbook::Workbook; // ── Pipeline (no UI state) ──────────────────────────────────────────────────── @@ -29,6 +30,9 @@ pub struct ImportPipeline { pub formulas: Vec, /// What to do with records that share every coordinate (improvise-cgqt). pub on_duplicate: DuplicatePolicy, + /// The files a CSV merge read, each of which gets its own view. Empty for + /// a single file or a JSON import (improvise-lpzu). + pub sources: Vec, } /// Several source records that land on one coordinate tuple (improvise-cgqt). @@ -64,6 +68,7 @@ impl ImportPipeline { model_name: "Imported Model".to_string(), formulas: vec![], on_duplicate: DuplicatePolicy::Refuse, + sources: vec![], }; // Auto-select if root is an array or there is exactly one candidate path. @@ -330,6 +335,10 @@ impl ImportPipeline { } } + for view in source_views(&wb, &self.sources, &plan) { + wb.views.insert(view.name.clone(), view); + } + let mut warnings: Vec = skip_warnings(self.records.len(), skipped_non_objects) .into_iter() .collect(); @@ -341,6 +350,60 @@ impl ImportPipeline { } } +/// One view per merged file, in file order, while `File` is a category. The +/// shape and why Filter: `context/code/formula-and-io.md` (improvise-lpzu). +fn source_views(wb: &Workbook, sources: &[SourceFile], plan: &Plan) -> Vec { + let file_is_a_category = plan.categories.iter().any(|c| c.field == FILE_CATEGORY); + let mut views: Vec = vec![]; + for source in sources.iter().filter(|_| file_is_a_category) { + let taken = |name: &str| { + wb.views.contains_key(name) + || VIRTUAL_VIEW_NAMES.contains(&name) + || views.iter().any(|v| v.name == name) + }; + let name = std::iter::once(source.item.clone()) + .chain((2usize..).map(|n| format!("{} {n}", source.item))) + .find(|name| !taken(name)) + .expect("a free name exists, there being finitely many views"); + views.push(source_view(wb, source, plan, name)); + } + views +} + +/// Filtered to the file; its own categories seated as a fresh import seats +/// them, every other one on None; other files' measures hidden. +fn source_view(wb: &Workbook, source: &SourceFile, plan: &Plan, name: String) -> View { + let has = |field: &str| source.columns.iter().any(|c| c == field); + let own = |cat: &str| { + has(cat) + || plan + .date_extractions + .iter() + .any(|(field, _, _, derived)| derived == cat && has(field)) + }; + let mut view = View::new(name); + for cat in wb.model.categories.values() { + match cat.kind.is_virtual() || own(&cat.name) { + true => view.on_category_added(&cat.name, cat.kind), + false => { + view.category_axes.insert(cat.name.clone(), Axis::None); + } + } + } + view.set_axis("_Measure", Axis::Page); + view.set_axis(FILE_CATEGORY, Axis::Filter); + view.set_page_selection(FILE_CATEGORY, &source.item); + let (mine, theirs): (Vec<&FieldProposal>, Vec<&FieldProposal>) = + plan.measures.iter().partition(|m| has(&m.field)); + for measure in theirs { + view.hide_item("_Measure", &measure.field); + } + if let Some(first) = mine.first() { + view.set_page_selection("_Measure", &first.field); + } + view +} + /// The accepted fields by role, and the date parts derived from them. struct Plan<'a> { categories: Vec<&'a FieldProposal>, @@ -581,8 +644,12 @@ pub struct ImportWizard { impl ImportWizard { pub fn new(raw: Value) -> Self { - let pipeline = ImportPipeline::new(raw); + Self::from_pipeline(ImportPipeline::new(raw)) + } + /// A wizard over a pipeline the caller built, such as a CSV merge that + /// carries its sources (improvise-lpzu). + pub fn from_pipeline(pipeline: ImportPipeline) -> Self { let step = if pipeline.needs_path_selection() { WizardStep::SelectArrayPath } else if pipeline.records.is_empty() { @@ -1068,6 +1135,7 @@ impl ImportWizard { mod tests { use super::{ImportPipeline, collision_warning}; use crate::import::analyzer::FieldKind; + use crate::view::Axis; use serde_json::{Value, json}; #[test] @@ -2096,28 +2164,141 @@ mod tests { assert!(warnings.is_empty(), "{warnings:?}"); } - /// code-maat's coupling and age reports, as a folder import merges them. - fn code_maat_records() -> Value { + /// CSV files as a folder import merges them, sources and all. + fn merged_pipeline(files: &[(&str, &str)]) -> ImportPipeline { use crate::import::csv_parser::merge_csvs_from; use crate::storage::{MemoryFs, Storage}; let fs = MemoryFs::new(); - let files = [ - ( - "maat/age.csv", - "entity,age-months\nsrc/a.rs,2\nsrc/c.rs,7\n", - ), - ( - "maat/coupling.csv", - "entity,coupled,degree,average-revs\nsrc/a.rs,src/b.rs,80,5\nsrc/b.rs,src/c.rs,40,3\n", - ), - ]; for (path, body) in files { fs.write(std::path::Path::new(path), body.as_bytes()) .unwrap(); } - let (records, warnings) = merge_csvs_from(&fs, &files.map(|(p, _)| p)).unwrap(); - assert!(warnings.is_empty(), "{warnings:?}"); - Value::Array(records) + let paths: Vec<&str> = files.iter().map(|(p, _)| *p).collect(); + let merged = merge_csvs_from(&fs, &paths).unwrap(); + assert!(merged.warnings.is_empty(), "{:?}", merged.warnings); + let mut p = ImportPipeline::new(Value::Array(merged.records)); + p.sources = merged.sources; + p + } + + /// code-maat's age and coupling reports. + const CODE_MAAT: [(&str, &str); 2] = [ + ( + "maat/age.csv", + "entity,age-months\nsrc/a.rs,2\nsrc/c.rs,7\n", + ), + ( + "maat/coupling.csv", + "entity,coupled,degree,average-revs\nsrc/a.rs,src/b.rs,80,5\nsrc/b.rs,src/c.rs,40,3\n", + ), + ]; + + fn code_maat_records() -> Value { + Value::Array(merged_pipeline(&CODE_MAAT).records) + } + + /// Every non-empty cell a view's grid draws, row by row. + fn drawn(wb: &crate::workbook::Workbook, view: &str) -> Vec { + use crate::view::GridLayout; + let layout = GridLayout::with_frozen_records(&wb.model, &wb.views[view], None); + (0..layout.row_count()) + .flat_map(|r| (0..layout.col_count()).map(move |c| (r, c))) + .filter_map(|(r, c)| layout.evaluate(&wb.model, &layout.cell_key(r, c)?)) + .filter_map(|v| v.as_f64()) + .collect() + } + + /// improvise-lpzu: each merged file gets a view of its own records and + /// columns. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_merge_gives_each_file_a_view_projected_to_its_columns() { + let (wb, _) = merged_pipeline(&CODE_MAAT) + .build_model_with_warnings() + .unwrap(); + assert_eq!( + wb.views.keys().collect::>(), + ["Default", "age", "coupling"] + ); + assert_eq!(wb.active_view, "Default"); + + let age = &wb.views["age"]; + assert_eq!(age.axis_of("File"), Axis::Filter); + assert_eq!(age.page_selection("File"), Some("age")); + assert_eq!(age.axis_of("entity"), Axis::Row); + assert_eq!(age.axis_of("coupled"), Axis::None); + assert_eq!(age.axis_of("_Measure"), Axis::Page); + assert_eq!(age.page_selection("_Measure"), Some("age-months")); + assert!(age.is_hidden("_Measure", "degree") && age.is_hidden("_Measure", "average-revs")); + assert!(!age.is_hidden("_Measure", "age-months")); + assert_eq!(drawn(&wb, "age"), [2.0, 7.0]); + + let coupling = &wb.views["coupling"]; + assert_eq!(coupling.page_selection("File"), Some("coupling")); + assert_eq!(coupling.axis_of("entity"), Axis::Row); + assert_eq!(coupling.axis_of("coupled"), Axis::Column); + assert_eq!(coupling.page_selection("_Measure"), Some("average-revs")); + assert!(coupling.is_hidden("_Measure", "age-months")); + assert!(!coupling.is_hidden("_Measure", "average-revs")); + assert_eq!(drawn(&wb, "coupling"), [5.0, 3.0]); + } + + /// improvise-lpzu: two files of one schema are told apart by the filter, + /// not by their columns. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_files_view_holds_only_that_files_records() { + let (wb, _) = merged_pipeline(&[ + ("s/q1.csv", "Region,Revenue\nEast,10\nWest,1\n"), + ("s/q2.csv", "Region,Revenue\nEast,20\nWest,2\n"), + ]) + .build_model_with_warnings() + .unwrap(); + assert_eq!(drawn(&wb, "q1"), [10.0, 1.0]); + assert_eq!(drawn(&wb, "q2"), [20.0, 2.0]); + } + + /// improvise-lpzu: a view name is taken from the file only while it is + /// free. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_files_view_name_skips_names_already_held() { + let (wb, _) = merged_pipeline(&[ + ("d/Default.csv", "Region,Revenue\nEast,1\n"), + ("d/_Drill.csv", "Region,Revenue\nEast,2\n"), + ]) + .build_model_with_warnings() + .unwrap(); + assert_eq!( + wb.views.keys().collect::>(), + ["Default", "Default 2", "_Drill 2"] + ); + assert_eq!( + wb.views["Default 2"].page_selection("File"), + Some("Default") + ); + assert_eq!(wb.views["_Drill 2"].page_selection("File"), Some("_Drill")); + } + + /// improvise-lpzu: the per-file views persist, and in the same order on + /// every import. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn per_file_views_survive_a_round_trip_and_repeat_byte_for_byte() { + use crate::persistence::{format_md, parse_md}; + let build = || { + let (wb, _) = merged_pipeline(&CODE_MAAT) + .build_model_with_warnings() + .unwrap(); + format_md(&wb) + }; + let text = build(); + assert_eq!(build(), text); + let reread = parse_md(&text).unwrap(); + assert_eq!(format_md(&reread), text); + assert_eq!( + reread.views.keys().collect::>(), + ["Default", "age", "coupling"] + ); + assert_eq!(reread.views["age"].axis_of("File"), Axis::Filter); + assert_eq!(reread.views["age"].page_selection("File"), Some("age")); + assert_eq!(drawn(&reread, "coupling"), [5.0, 3.0]); } /// improvise-lpzu: a column a file lacks is absent from its records, never diff --git a/src/draw.rs b/src/draw.rs index 65a8eb52..158e674f 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -58,7 +58,7 @@ impl<'a> Drop for TuiContext<'a> { pub fn run_tui( workbook: crate::workbook::Workbook, file_path: Option, - import_value: Option, + import: Option, ) -> Result<()> { let mut stdout = io::stdout(); let mut tui_context = TuiContext::enter(&mut stdout)?; @@ -68,8 +68,8 @@ pub fn run_tui( app.term_height = h; } - if let Some(json) = import_value { - app.start_import_wizard(json); + if let Some(pipeline) = import { + app.start_import(pipeline); } else if app.is_empty_model() { app.view_state.mode = AppMode::Help; } diff --git a/src/main.rs b/src/main.rs index 04dc835d..cdd3b83f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -144,7 +144,7 @@ impl Runnable for ImportArgs { if self.files.is_empty() { anyhow::bail!("No files specified for import"); } - let import_value = get_import_data(&self.files) + let pipeline = get_import_data(&self.files) .ok_or_else(|| anyhow::anyhow!("Failed to parse import files"))?; let config = ImportConfig { @@ -160,9 +160,9 @@ impl Runnable for ImportArgs { }; if self.no_wizard { - run_headless_import(import_value, &config, self.output, model_file) + run_headless_import(pipeline, &config, self.output, model_file) } else { - run_wizard_import(import_value, &config, model_file) + run_wizard_import(pipeline, &config, model_file) } } } @@ -202,7 +202,7 @@ fn parse_colon_pairs(args: &[String]) -> Vec<(String, String)> { .collect() } -use import::wizard::DuplicatePolicy; +use import::wizard::{DuplicatePolicy, ImportPipeline}; fn apply_config_to_pipeline(pipeline: &mut import::wizard::ImportPipeline, config: &ImportConfig) { use import::analyzer::{DateComponent, FieldKind}; @@ -289,12 +289,11 @@ fn apply_axis_overrides(wb: &mut Workbook, axes: &[(String, String)]) { } fn run_headless_import( - import_value: Value, + mut pipeline: ImportPipeline, config: &ImportConfig, output: Option, model_file: Option, ) -> Result<()> { - let mut pipeline = import::wizard::ImportPipeline::new(import_value); apply_config_to_pipeline(&mut pipeline, config); if pipeline.on_duplicate == DuplicatePolicy::Refuse { let collisions = pipeline.collisions(); @@ -318,19 +317,19 @@ fn run_headless_import( } fn run_wizard_import( - import_value: Value, + pipeline: ImportPipeline, _config: &ImportConfig, model_file: Option, ) -> Result<()> { let workbook = get_initial_workbook(&model_file)?; // The wizard configures itself inside the TUI. // TODO: pass `_config` to it for pre-population. - run_tui(workbook, model_file, Some(import_value)) + run_tui(workbook, model_file, Some(pipeline)) } // ── Import data loading ────────────────────────────────────────────────────── -fn get_import_data(args: &[PathBuf]) -> Option { +fn get_import_data(args: &[PathBuf]) -> Option { use improvise::import::csv_parser::{ImportPath, expand_import_paths}; // Whether an argument is a folder is the one filesystem question asked here. let classified: Vec = args @@ -355,10 +354,18 @@ fn get_import_data(args: &[PathBuf]) -> Option { eprintln!("Multi-file import only supports CSV files"); return None; } - match crate::import::csv_parser::merge_csvs(paths) { - Ok(records) => Some(Value::Array(records)), + let fs = improvise_io::storage::PhysicalFs; + match crate::import::csv_parser::merge_csvs_from(&fs, paths) { + Ok(merged) => { + for warning in &merged.warnings { + eprintln!("warning: {warning}"); + } + let mut pipeline = ImportPipeline::new(Value::Array(merged.records)); + pipeline.sources = merged.sources; + Some(pipeline) + } Err(e) => { - eprintln!("CSV merge error: {e}"); + eprintln!("CSV merge error: {e:#}"); None } } @@ -372,7 +379,7 @@ fn get_import_data(args: &[PathBuf]) -> Option { Ok(content) => { if csv_path_p(path) { match crate::import::csv_parser::parse_csv(path) { - Ok(records) => Some(Value::Array(records)), + Ok(records) => Some(ImportPipeline::new(Value::Array(records))), Err(e) => { eprintln!("CSV parse error: {e}"); None @@ -384,7 +391,7 @@ fn get_import_data(args: &[PathBuf]) -> Option { eprintln!("JSON parse error: {e}"); None } - Ok(json) => Some(json), + Ok(json) => Some(ImportPipeline::new(json)), } } } diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index 4d4bdeec..b1f33267 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -653,6 +653,18 @@ fn a_headless_import_of_a_folder_merges_its_csv_files_and_repeats_byte_for_byte( assert!(doc.lines().any(|l| l == line), "{line} missing from\n{doc}"); } assert!(!doc.contains("README"), "{doc}"); + let views: Vec<&str> = doc.lines().filter(|l| l.starts_with("## View: ")).collect(); + assert_eq!( + views, + [ + "## View: Default", + "## View: abs-churn", + "## View: age", + "## View: coupling" + ] + ); + assert!(doc.lines().any(|l| l == "File: filter, coupling"), "{doc}"); + assert!(doc.contains("Initial View: Default"), "{doc}"); // The output now sits in the folder; a second run must not read it. let second = run_import(&dir, &out_path, &[]); From 736e9e5e186b883341a3be78cb7a150264b3f69c Mon Sep 17 00:00:00 2001 From: "Claude (agent-lpzu)" Date: Thu, 24 Sep 2026 20:51:40 -0700 Subject: [PATCH 3/6] test(import): pin where an absent coordinate draws and how a saved file view projects (improvise-8jk6.2, improvise-lpzu) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-io/src/import/wizard.rs | 55 +++++++++++++++++++++++ tests/folder_import_views.rs | 57 ++++++++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 tests/folder_import_views.rs diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index d3283849..232e5e19 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -2208,6 +2208,61 @@ mod tests { .collect() } + /// improvise-8jk6.2: where a record with no `Date` shows up, axis by axis. + /// Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_record_with_no_coordinate_is_drawn_only_where_that_category_is_off_the_grid() { + use crate::model::cell::CellKey; + use crate::persistence::{format_md, parse_md}; + use crate::view::{GridLayout, drill_snapshot}; + let mut p = ImportPipeline::new(json!([ + {"Payee": "Acme", "Date": "d1", "Amount": 10.0}, + {"Payee": "Bolt", "Amount": 20.0}, + ])); + force_category(&mut p, "Date"); + let (mut wb, _) = p.build_model_with_warnings().unwrap(); + let mut seat = |date: Axis, payee: Axis| { + let view = wb.active_view_mut(); + view.set_axis("_Measure", Axis::None); + view.set_axis("Payee", payee); + view.set_axis("Date", date); + (drawn(&wb, "Default"), wb.clone()) + }; + assert_eq!( + seat(Axis::Column, Axis::Row).0, + [10.0], + "no column holds Bolt" + ); + assert_eq!(seat(Axis::Page, Axis::Row).0, [10.0], "no page holds Bolt"); + assert_eq!( + seat(Axis::None, Axis::Row).0, + [10.0, 20.0], + "None counts it" + ); + + let (_, mut records) = seat(Axis::None, Axis::None); + records.active_view_mut().set_axis("_Index", Axis::Row); + records.active_view_mut().set_axis("_Dim", Axis::Column); + let layout = GridLayout::with_frozen_records(&records.model, records.active_view(), None); + let row: Vec = (0..layout.col_count()) + .map(|c| layout.records_display(1, c).unwrap_or_default()) + .collect(); + assert_eq!(row, ["", "Bolt", "", "20", ""], "a blank Date cell"); + + let text = format_md(&records); + assert!( + text.contains("\nPayee=Bolt, _Measure=Amount = 20\n"), + "{text}" + ); + assert_eq!(format_md(&parse_md(&text).unwrap()), text); + + let drill = |cat: &str, item: &str| { + let key = CellKey::new(vec![(cat.to_string(), item.to_string())]); + drill_snapshot(&records.model, &key).len() + }; + assert_eq!((drill("Payee", "Bolt"), drill("Date", "d1")), (1, 1)); + } + /// improvise-lpzu: each merged file gets a view of its own records and /// columns. Evidence: `context/test-evidence/formula-and-io.md`. #[test] diff --git a/tests/folder_import_views.rs b/tests/folder_import_views.rs new file mode 100644 index 00000000..43a8d0f4 --- /dev/null +++ b/tests/folder_import_views.rs @@ -0,0 +1,57 @@ +//! A folder import's per-file views, saved and reloaded, as both front ends +//! project them (improvise-lpzu). Evidence: `context/test-evidence/cli.md`. + +use improvise::import::csv_parser::merge_csvs_from; +use improvise::import::wizard::ImportPipeline; +use improvise::persistence::{format_md, parse_md}; +use improvise::ui::app::App; +use improvise_command::ui::project::{Viewport, grid_in}; +use improvise_io::storage::{MemoryFs, Storage}; + +fn reloaded_folder_import() -> App { + let fs = MemoryFs::new(); + let files = [ + ( + "maat/age.csv", + "entity,age-months\nsrc/a.rs,2\nsrc/c.rs,7\n", + ), + ( + "maat/coupling.csv", + "entity,coupled,degree,average-revs\nsrc/a.rs,src/b.rs,80,5\nsrc/b.rs,src/c.rs,40,3\n", + ), + ]; + for (path, body) in files { + fs.write(std::path::Path::new(path), body.as_bytes()) + .unwrap(); + } + let merged = merge_csvs_from(&fs, &files.map(|(p, _)| p)).unwrap(); + let mut pipeline = ImportPipeline::new(serde_json::Value::Array(merged.records)); + pipeline.sources = merged.sources; + let (wb, _) = pipeline.build_model_with_warnings().unwrap(); + App::new(parse_md(&format_md(&wb)).unwrap(), None) +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_saved_per_file_view_projects_only_its_file() { + let app = reloaded_folder_import(); + let whole = Viewport { + row_offset: 0, + col_offset: 0, + rows: 100, + cols: 100, + }; + for (view, texts) in [("age", ["2.00", "7.00"]), ("coupling", ["5.00", "3.00"])] { + let grid = grid_in(&app, view, whole); + let file = grid.page_coords.iter().find(|p| p.cat == "File").unwrap(); + assert_eq!((file.selected.as_str(), &file.items), (view, &None)); + let drawn: Vec<&str> = grid + .cells + .iter() + .flatten() + .map(|c| c.text.as_str()) + .filter(|t| !t.is_empty()) + .collect(); + assert_eq!(drawn, texts, "{view}"); + } +} From ee0ff83a58af0de2c55350772fca352403c35fa6 Mon Sep 17 00:00:00 2001 From: "Claude (agent-lpzu)" Date: Thu, 24 Sep 2026 20:58:58 -0700 Subject: [PATCH 4/6] docs(import): folder import, sparse records and per-file views (improvise-lpzu, improvise-8jk6.2) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- README.md | 9 +- context/code/cli.md | 21 +++ context/code/formula-and-io.md | 117 ++++++++++++++++- context/test-evidence/cli.md | 38 ++++++ context/test-evidence/formula-and-io.md | 162 +++++++++++++++++++++++- 5 files changed, 335 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 11e6fa19..95094b06 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,14 @@ opening one fails with an error that says so. JSON *import* is unaffected. The import wizard analyzes CSV columns and proposes each as a category (string-valued), measure (numeric), time dimension (date-like, with optional year/month/quarter extraction), or skip. Multiple CSVs merge automatically -with a synthetic "File" category. +with a synthetic "File" category, even when their headers differ. A record +simply has no value for a column its file lacks. A folder argument imports +every `*.csv` in it, in name order, and each merged file also gets its own view, +filtered to that file and showing only its columns: + +```sh +improvise import reports/ --no-wizard -o reports.improv +``` All model mutations go through a typed command registry, so the same operations that back the TUI work headless: diff --git a/context/code/cli.md b/context/code/cli.md index 5b5ab034..c9f6fec2 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -369,3 +369,24 @@ non-zero and writes nothing. It is checked before the build rather than read off the build's error, so the listing is structured data and not a parsed sentence. `separate` uses `ImportPipeline::suggested_duplicate_category`, and there is deliberately no flag to name the category yet. + +## 12 — `import` takes folders, and the CLI only says which arguments are folders (improvise-lpzu) + +`improvise import` takes any mix of files and folders. A folder stands for its +`*.csv` files, in name order, so `import dir/` is `import dir/*.csv`. The rules +and their reasons are in `context/code/formula-and-io.md`, "A folder of CSVs is +one sparse model, with a view per file". The CLI's part is the one question +`Storage` cannot answer: whether a path is a folder. `get_import_data` asks +`Path::is_dir` for each argument, hands the resulting `ImportPath`s to +`expand_import_paths` over `PhysicalFs`, and from there takes the path a list +of files always took. A folder with no CSV in it is refused before anything is +read. The message names the folder, and the run exits non-zero and writes +nothing. + +A merge now reaches the build as an `ImportPipeline` carrying the merged files' +`sources`, rather than as a bare JSON value. Both `--no-wizard` and the TUI +wizard (`run_tui` → `App::start_import`) therefore build the per-file views. +The headless path matters most, because the code-maat pipeline +(improvise-1xfb.5.1) runs there. `a_headless_import_of_a_folder_merges_its_csv_files_and_repeats_byte_for_byte` +pins the views, the sparse Data lines and the byte-identical second run, with +the first run's output sitting in the folder it reads. diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 2d07d8e6..90c8563f 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -632,6 +632,88 @@ one proves nothing. backend rather than only a file (improvise-fuj); the path-taking wrappers remain for the native CLI. +### A folder of CSVs is one sparse model, with a view per file (improvise-lpzu) + +`improvise import ` exists for improvise-1xfb.5.1. code-maat writes one +CSV per analysis into a directory, and the schemas differ: coupling has +`entity,coupled,degree,average-revs`, age has `entity,age-months`, and +abs-churn has `date,added,deleted,commits`. The user ruled (2026-09-25): +"Records are already sparse: the model can just leave null/empty values for +columns not in a given CSV file." So files with different headers merge into +**one** model. It is not refused, and it is not split into one model per schema. + +**A folder argument means exactly its `*.csv` files.** `expand_import_paths` +takes the arguments in order and replaces each folder with its `*.csv` +entries. The extension check is case-insensitive, the listing is not recursive, +and the order is `Storage::list`'s, which is sorted by contract. Then the +existing path runs unchanged: two or more files merge through `merge_csvs_from` +with a `File` category, and one file imports alone. So `import dir/` is +`import dir/*.csv`, including when the folder holds a single CSV. Four cases +follow from that equivalence rather than being chosen one by one: + +- **Non-CSV entries are skipped**, not refused, because a glob skips them. This + includes the output `.improv`, which a second run finds in the folder. +- **A folder with no CSV at all is refused**, naming the folder. A glob that + matches nothing is an error too, and importing nothing would write an empty + document. +- **Files and folders mix freely**, in argument order. +- **A file named twice is read twice.** For example, `dir/a.csv dir/` reads + `a.csv` twice, and its records collide with themselves. The headless path + refuses that like any other collision. Nothing deduplicates, because the + shell glob would not either. + +The listing goes through `Storage::list` so the browser host can reuse it +(gotcha 2). `Storage` knows documents, not folders, so **whether an argument +is a folder is the caller's question**. The CLI answers it at the edge with +`Path::is_dir` and passes an `ImportPath::{File, Folder}`. An OPFS host would +answer it from its own UI. + +**Sparse merge.** A record from a file without some column has no key for it, +so `Plan::place` gives it no coordinate there (improvise-8jk6.2 above), and no +measure cell. Nothing is fabricated: no `0` and no `""`. The analyzer already +classified each column over the values present (improvise-l1f5), so a column +only one file has is judged by that file's values alone. The category cap +counts the **union** of every file's categories. Rows from different files +cannot collide, because `File` differs. + +**Each file gets a view** (the user, 2026-09-25: "dir import should do this: +one view filtered by the file column and projected to the columns in that +file"). `merge_csvs_from` returns a `Merged` whose `sources` list each `File` +item with the columns its header named. Two files with one stem share an item, +so their columns are unioned. The pipeline carries `sources`, and the build +adds one view per source after the merged `Default` view, which stays the +initial view. Each per-file view: + +- **Is named after the file's stem.** A name already taken, including the + virtual `_Records` and `_Drill`, gets ` 2`, ` 3` and so on. A view named + `_Drill` would never be written and would be dropped on load. +- **Puts `File` on `Axis::Filter`, fixed at that file's item.** Filter and + not Page, because the view *is* that file's projection. Paging to another + file would show that file's records through this file's columns and hidden + measures, which is wrong on every page but one. Filter constrains without + being navigable (gotcha 27). Until now it appeared only in the never-saved + `_Drill` view, but the `.improv` grammar has always round-tripped it. + `per_file_views_survive_a_round_trip_and_repeat_byte_for_byte` proves that on + a saved per-file view, and `a_saved_per_file_view_projects_only_its_file` + proves the shared `grid_in` projection, which both the TUI and the browser + draw from, carries it as a fixed coordinate with nothing to cycle. +- **Seats the file's own categories** (its columns, plus date parts derived + from them) through `View::on_category_added`, in model order: the rule a + single-file import uses. Every other category goes on `None`. The file's + records lack those, so nothing splits. +- **Puts `_Measure` on Page**, as `Workbook::new` does, **with other files' + measures hidden** and the page selection on the file's first measure. A + formula target comes from no file, so it stays visible. + +A per-file view is made only while `File` is an accepted category. A user who +skipped or reclassified it has no filter to build views from. + +Two things this does not do. Page cycling still steps onto a hidden item and +draws an empty grid: `[`/`]` in a per-file view can reach another file's +measure (improvise-yd2t; the Default view already behaved this way). And a +CSV with its own `File` column has it overwritten by the merge's, which is the +existing behaviour of `merge_csvs`. + ### The wizard's back path is a history stack `advance` pushes the step it leaves; `previous` pops it. Keeping the path @@ -658,10 +740,33 @@ and a blank string are **absences, not names**; stringifying them used to invent an item literally called "null" (improvise-ych). Every other value keeps its printed form. -A record missing a value for an accepted Category field has no place in the grid -and is dropped; `build_model_with_warnings` counts it against the *first* -category it is missing and reports the total, because without a warning that -loss is invisible in a thousand-row import (improvise-5j9). +**A record with no value for a category keeps its cell, without that +coordinate** (improvise-8jk6.2). Records are sparse: a `CellKey` names only the +coordinates it has. So a missing field, a `null` and a blank string all mean +"no coordinate here". None of them makes an item: not `"null"`, not `""`, and +no synthetic placeholder. The same rule covers labels, which used to become an +item named `""`. The user's words, 2026-09-25: "this limit in import should be +fixed, it dates to when this wasn't sparse." That limit was improvise-5j9's +rule: such a record "has no place in the grid" and was dropped, counted and +reported as `Skipped N of M records missing category values`. 5j9's warning +made the loss visible, and the loss is now gone, so the warning went with it. +The only record still skipped is one that is not a JSON object. + +Where such a record shows up is decided by the axes, not by the import. The +test is `a_record_with_no_coordinate_is_drawn_only_where_that_category_is_off_the_grid`: + +- **The category on Row or Column:** no position holds the record. Its other + coordinates may still draw a row, but the cell under the category's items is + empty, and the column total leaves it out. +- **The category on Page:** no selection shows it. +- **The category on None:** it is aggregated like any other record. +- **Records mode:** its row shows a blank cell in that category's column. +- **`.improv`:** its Data line omits the category, and it round-trips. +- **Drill:** a drill that does not constrain the category reaches it. + +Whether Row/Column/Page should give such records a place of their own (a +"(none)" position) is **open, and it is the user's call**. It is raised on the +lpzu PR. Nothing here invents one. ### Colliding records are a question, not a default (improvise-cgqt) @@ -678,7 +783,9 @@ made it choose too. It chose `Refuse` and points at `:import`. **Detection comes before the build and shares the build's placement.** Where a record lands is `Plan::place`: its categories, then its date parts, then its -labels, then its measure values, or a reason it is dropped. `collisions()` and +labels, then its measure values. Only a record that is not an object is +dropped, since a missing value is no coordinate rather than a reason to drop +(improvise-8jk6.2). `collisions()` and `build_model_with_warnings` both call it, so the question the wizard asks and the thing the build does cannot disagree. Neither has a second walk of the record. **A collision is a cell that more than one record writes**, meaning a diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index be0c036f..424792e3 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -407,3 +407,41 @@ named, and no output file. It has two siblings. `..._sums_duplicates_when_told_t asserts `= 30` and no `Occurrence`. `..._keeps_duplicates_separate_when_told_to` asserts the category, `= 10` and `= 20`, and no `= 30`. So the two flags cannot both be summing. + +### a_headless_import_of_a_folder_merges_its_csv_files_and_repeats_byte_for_byte + +improvise-lpzu, at the process boundary. The folder holds code-maat's three +schemas (coupling, age and abs-churn) and a `README.txt`. At `e65d87fd`'s +parent the run failed with `Cannot read '…/maat': Is a directory (os error +21)`, because a single path went straight to `read_to_string`. The test +asserts: + +- sparse Data lines for all three schemas, with no README; +- the four views, `Default` first and initial, then one per file; +- `File: filter, coupling`; +- a second run, with the first run's `maat.improv` now inside the folder, + writing the same bytes. + +Rivals R2, R5, R6 and R7 each fail it. + +### a_headless_import_takes_files_and_folders_together_and_refuses_an_empty_folder + +improvise-lpzu. A file argument and a folder argument together are one merge, +and the extra file's records carry its own `File` item. Before the fix this +failed with `Multi-file import only supports CSV files`, because the folder was +not a `.csv` path. An empty folder exits non-zero, writes nothing, and names +itself: `no CSV files in folder …/empty`. Rivals R2, R4 and R5 each fail it. + +### a_saved_per_file_view_projects_only_its_file + +improvise-lpzu (`tests/folder_import_views.rs`). A folder import is saved, +reloaded and projected through `project::grid_in`, the one derivation the TUI +grid and the browser both draw from (gotcha 44). This is why no separate TUI +or browser test was written. For each file's view: + +- the page bar carries `File` fixed at that file with nothing to cycle + (`items: None`, gotcha 27); +- the drawn cells are that file's alone: `2.00, 7.00` for age and + `5.00, 3.00` for coupling. + +Rivals R2, R7 and R9 each fail it. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 3ef8badc..71710e03 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1026,6 +1026,14 @@ every blank column value in a CSV import grew a phantom "null" item. Null means *absent*: the record must be skipped and counted, exactly as if the key were missing. +**The skip is superseded by improvise-8jk6.2.** Null still means absent, and +still invents no item. But an absent coordinate no longer drops the record: it +keeps its cell without that coordinate. So the test now asserts both cells +(`region=East` and the bare `_Measure=revenue`) and no warning. The rival that +restores the skip (a present-but-blank category returns the record as +unplaceable) fails here, beside +`a_record_with_no_value_for_a_category_is_kept_without_that_coordinate`. + ### build_model_literal_string_null_is_a_real_category_item The other side of improvise-ych: the *string* `"null"` is an ordinary value — a @@ -1038,7 +1046,9 @@ null" would pass the test above. improvise-ych, third arm. An empty string is the JSON spelling of a blank cell — the CSV parser already turns a blank field into `Value::Null`, so the two inputs must agree. An item with an empty name is unnameable in the grid, so it is -treated as missing rather than created. +treated as missing rather than created. Since improvise-8jk6.2 "missing" +means the record keeps its cell without a `region` coordinate. It is no longer +skipped, and the same skip-restoring rival fails here. ### build_model_sums_records_that_share_every_coordinate @@ -1107,12 +1117,19 @@ and scrolls them. It was red at compile time against the old `Option` signature, then red against a `Vec` that still held one elided sentence. The rival **elided after three** fails it. -### build_model_warns_about_records_skipped_for_missing_category +### improvise-5j9's skip warning, retired by improvise-8jk6.2 improvise-5j9: skipping a record for a missing category value used to be silent, -so a 1000-row import could drop rows with no user-visible sign. The warning must -name the count, the total, and the offending field(s) — a bare "some records were -skipped" is not actionable. +so a 1000-row import could drop rows with no user-visible sign. The warning had +to name the count, the total, and the offending field(s); a bare "some records +were skipped" is not actionable. Its test, +`build_model_warns_about_records_skipped_for_missing_category`, was **deleted** +by improvise-8jk6.2. A record missing a category value is now kept without that +coordinate, so there is no skip left to report. Its successor is +`a_record_with_no_value_for_a_category_is_kept_without_that_coordinate`, which +asserts no warning at all. `build_model_record_with_missing_category_value_skipped` +became `build_model_record_with_missing_category_field_is_kept` and now asserts +the kept cell rather than only the East one. ### is_does_not_swallow_the_start_of_a_longer_word @@ -1436,7 +1453,16 @@ The fix reports and does not relocate: the row still lands with the coordinates it does have, exactly as before. Changing where such a row is placed is a behaviour change the ticket did not ask for. -### import::wizard::tests::a_row_dropped_for_a_missing_category_is_not_charged_to_the_date_tally +### import::wizard::tests::a_row_kept_without_a_category_is_charged_to_the_date_tally + +**Renamed from `a_row_dropped_for_a_missing_category_is_not_charged_to_the_date_tally` +by improvise-8jk6.2.** Its premise went with the skip. A row missing `Region` +now lands, so its unparseable `Date` *is* a derived column's loss, and it is +charged once to each of `Date_Month` and `Date_Year`. The test asserts the +landed cell (`Date=2025-02-05, _Measure=Amount = 200`, with no `Region` and no +derived part) and exactly those two warnings. The history below is kept +because the rule it established (charge only landed rows) still holds. Every +object row now lands. improvise-yss5, found by ChatGPT Codex reviewing PR #150 at `af7fae0e` and correct. The first fix incremented `unparsed_dates` while walking a record's @@ -1617,3 +1643,127 @@ that name, which must be refused. It was green on arrival, so its evidence is the reviewer's rival, **date-derived names dropped from the taken names**, which fails it. That fault would have let `add_category` reuse the existing category, and every cell key would then name `when_Year` twice. + +### a_record_with_no_value_for_a_category_is_kept_without_that_coordinate + +improvise-8jk6.2, reported by the user from the wizard: "Skipped 6 of 342 +records missing category values: Settlement Date (6)". `Plan::place` returned +`Placement::Missing` for a category with no value, so the build dropped the +whole record, measures and all. The fixture has one complete record, one with +`"Settlement Date": null` and one with `""`, for two different payees. At +`e65d87fd`'s parent the build stored only the first cell. Now the stored cells +are exactly the three records, the two blanks with no date coordinate, and +there is no warning. `Settlement Date` holds only its one real item, so neither +`"null"` nor `""` became one. Rival R1, a record with a present-but-blank +category treated as unplaceable, fails this test and the two improvise-ych +tests. + +### files_with_different_headers_import_as_one_sparse_model + +improvise-lpzu. code-maat's `age.csv` (`entity,age-months`) and `coupling.csv` +(`entity,coupled,degree,average-revs`) merged through `merge_csvs_from`. Before +the fix, the age records had no `coupled` key, so they took the same `Missing` +path and **every age record was dropped**. The build stored the four coupling +cells and nothing else. The test pins the proposals (each column's kind, +judged over its present values), all six stored cells sorted, no empty-named +item in any category, and no warning. Rival R2, a record whose file lacks a +category column skipped, fails it along with the two CLI folder tests, the +projection test and the per-file view test. + +### a_label_column_a_record_lacks_is_no_coordinate_not_an_empty_item + +improvise-lpzu, the label half. Labels were attached with +`scalar_name(..).unwrap_or_default()`, so an age record, whose file has no +`coupled`, stored `coupled=` as a coordinate, and `coupled` grew an item named +`""`. Before the fix the first sorted cell was +`File=age, _Measure=age-months, coupled=, entity=src/a.rs = 2`. Rival R3 +(`unwrap_or_default` restored) fails it. + +### a_folder_expands_to_its_csv_files_in_name_order_and_skips_the_rest + +improvise-lpzu. The folder holds three CSVs written out of name order, one +with an upper-case `.CSV`, plus a `.txt`, an `.improv` (the output a second run +finds) and a CSV in *another* folder. Only the three are returned, sorted. +`MemoryFs` stores in a `HashMap`, so the order comes from `Storage::list`'s +contract and not from insertion. Rival R5 (the extension filter dropped) and +rival R6 (the listing passed through a `HashSet`) each fail it. + +### a_folder_with_no_csv_files_is_refused_by_name + +improvise-lpzu. Both an empty folder and a folder holding only non-CSV files +must refuse with `no CSV files in folder maat`. Importing nothing would write +an empty document. Rival R4 (the `ensure!` removed) fails it, and so does R5, +because the text-only folder then "has" files. + +### files_and_folders_expand_in_argument_order + +improvise-lpzu. `[z.csv, b/, a.csv]` must expand to +`[z.csv, b/one.csv, b/two.csv, a.csv]`. Argument order is kept, and a folder +is sorted within itself. A fixture listing only folders, or files in name +order, could not tell "argument order" from "sort everything". R6 fails it. + +### files_with_different_headers_merge_without_inventing_the_missing_columns + +improvise-lpzu, at the parser. Each merged record carries exactly its own +file's keys plus `File`: no key, `null` or `0` for a column another file has. +Each `SourceFile` lists its file's columns in header order. The per-file views +are built from those lists. + +### a_record_with_no_coordinate_is_drawn_only_where_that_category_is_off_the_grid + +improvise-8jk6.2's downstream question, asked of the real layout rather than +argued. Bolt has no `Date`: + +- With `Date` on Column or on Page, the grid draws only Acme's `10`. +- With `Date` on None, it draws both `10` and `20`. +- In records mode, Bolt's row reads `["", "Bolt", "", "20", ""]`, a blank + `Date` cell. +- The Data line is `Payee=Bolt, _Measure=Amount = 20`, and it round-trips. +- A drill on `Payee=Bolt` reaches the record. + +This is a characterization of the model's existing sparse semantics. It found +no bug. Its purpose is to make the Row/Column answer ("invisible") a stated +fact the user can overrule, rather than an accident. R2 fails it. + +### a_merge_gives_each_file_a_view_projected_to_its_columns + +improvise-lpzu, the user's "one view filtered by the file column and +projected to the columns in that file". The test asserts: + +- the view order (`Default`, then the files in read order), with `Default` + still active; +- `File` on Filter at the file's item; +- the file's own categories seated, and the other file's on None; +- `_Measure` on Page at the file's first measure, with the other file's + measures hidden; +- the drawn values. + +The age view draws `[2, 7]`. The coupling view draws `[5, 3]`, because its +first measure in model order is `average-revs`: serde_json's map orders keys +alphabetically. Failed at `bb505f8f`'s parent, where no view existed. Rivals +R7 (File on None), R9 (every category seated) and R10 (nothing hidden) each +fail it. + +### a_files_view_holds_only_that_files_records + +improvise-lpzu. The code-maat fixture cannot show that the *filter* works, +because its files share no measure, so the measure page alone would isolate +them. Two quarterly files with the same `Region,Revenue` schema can: without +the filter, `q1` would draw `30` and `3`. Rival R7 fails it. R9 and R10 do +not, and are not meant to. + +### a_files_view_name_skips_names_already_held + +improvise-lpzu. Two stems collide with names already held: `Default`, the +merged view, and `_Drill`, a virtual view that is never written and is dropped +on load. The first becomes `Default 2` and the second `_Drill 2`, and each +still filters to its own item. Rival R8 (the virtual names not checked) fails +it: the file's view would have been saved as `_Drill` and lost. + +### per_file_views_survive_a_round_trip_and_repeat_byte_for_byte + +improvise-lpzu. Two builds from two independently written `MemoryFs` give +identical `.improv` text, and `format(parse(text)) == text`. The reloaded +document still has the three views in order, `File: filter` at the file's item, +and the same drawn values. This is the first persisted, non-virtual use of +`Axis::Filter` (gotcha 27). R7 and R9 fail it. From 005b7a769a55f4ce4753a46cf17219881024bd25 Mon Sep 17 00:00:00 2001 From: "Claude (agent-lpzu)" Date: Thu, 24 Sep 2026 21:36:00 -0700 Subject: [PATCH 5/6] feat(import): a folder of one CSV keeps its File category and view (improvise-lpzu) The user's ruling on PR #314: a folder always merges, so even a single CSV in it records its file identity. A CSV named directly still imports alone. The "(none)" question is answered as no position, per the user. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- README.md | 4 ++-- context/code/cli.md | 4 +++- context/code/formula-and-io.md | 34 +++++++++++++++++++++++++++------- context/test-evidence/cli.md | 11 +++++++++++ src/main.rs | 6 +++++- tests/headless_cli.rs | 31 +++++++++++++++++++++++++++++++ 6 files changed, 79 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 95094b06..86080235 100644 --- a/README.md +++ b/README.md @@ -180,8 +180,8 @@ The import wizard analyzes CSV columns and proposes each as a category year/month/quarter extraction), or skip. Multiple CSVs merge automatically with a synthetic "File" category, even when their headers differ. A record simply has no value for a column its file lacks. A folder argument imports -every `*.csv` in it, in name order, and each merged file also gets its own view, -filtered to that file and showing only its columns: +every `*.csv` in it, in name order, even when it holds a single CSV. Each file +also gets its own view, filtered to that file and showing only its columns: ```sh improvise import reports/ --no-wizard -o reports.improv diff --git a/context/code/cli.md b/context/code/cli.md index c9f6fec2..8fecbd1d 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -373,7 +373,9 @@ there is deliberately no flag to name the category yet. ## 12 — `import` takes folders, and the CLI only says which arguments are folders (improvise-lpzu) `improvise import` takes any mix of files and folders. A folder stands for its -`*.csv` files, in name order, so `import dir/` is `import dir/*.csv`. The rules +`*.csv` files, in name order. A folder always merges, even when it holds a +single CSV, so that file keeps its `File` item and view. A single file named +directly still imports alone (the user's ruling on the PR). The rules and their reasons are in `context/code/formula-and-io.md`, "A folder of CSVs is one sparse model, with a view per file". The CLI's part is the one question `Storage` cannot answer: whether a path is a folder. `get_import_data` asks diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 90c8563f..db85de87 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -646,10 +646,28 @@ columns not in a given CSV file." So files with different headers merge into takes the arguments in order and replaces each folder with its `*.csv` entries. The extension check is case-insensitive, the listing is not recursive, and the order is `Storage::list`'s, which is sorted by contract. Then the -existing path runs unchanged: two or more files merge through `merge_csvs_from` -with a `File` category, and one file imports alone. So `import dir/` is -`import dir/*.csv`, including when the folder holds a single CSV. Four cases -follow from that equivalence rather than being chosen one by one: +existing path runs, with one difference: **a folder always merges**. Two or more +files merge through `merge_csvs_from` with a `File` category, and so does a +folder holding a single CSV. That gives the one file its `File` item and its +per-file view. + +The single-CSV rule is **the user's ruling (2026-09-25)**, which replaced this +PR's first answer, "`import dir/` is exactly `import dir/*.csv`". Their words: +"yes, we might want to have some kind of update import eventually (don't plan +this yet, but we'd want file identity for this)". A folder names a collection +that may grow, so a document built from it should record which file each +record came from from the start. + +**A single CSV named directly, not through a folder, still imports alone, +without `File`.** That is deliberate. Naming one file asks for that file, not +for a collection, and the shipped recipes (`examples/demo.improv` from +`examples/demo.csv`) and every existing single-file script would otherwise gain +a `File` category and a second view. File identity for update-import can be +asked for by naming a folder. The test is +`a_folder_of_one_csv_still_gets_a_file_category_and_its_view_but_a_lone_file_does_not`. + +Apart from that, a folder still behaves as the glob would, and four cases follow +from that rather than being chosen one by one: - **Non-CSV entries are skipped**, not refused, because a glob skips them. This includes the output `.improv`, which a second run finds in the folder. @@ -764,9 +782,11 @@ test is `a_record_with_no_coordinate_is_drawn_only_where_that_category_is_off_th - **`.improv`:** its Data line omits the category, and it round-trips. - **Drill:** a drill that does not constrain the category reaches it. -Whether Row/Column/Page should give such records a place of their own (a -"(none)" position) is **open, and it is the user's call**. It is raised on the -lpzu PR. Nothing here invents one. +**No "(none)" position, by the user's decision (2026-09-25).** They were asked +whether Row, Column or Page should give such records a place of their own. Their +answer: "I think leaving it out for now is fine, maybe a UI element somewhere +that shows the number of records underneath the current view?" That indicator +is improvise-gtn7.10, and it is not an import concern. ### Colliding records are a question, not a default (improvise-cgqt) diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 424792e3..1145ee78 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -445,3 +445,14 @@ or browser test was written. For each file's view: `5.00, 3.00` for coupling. Rivals R2, R7 and R9 each fail it. + +### a_folder_of_one_csv_still_gets_a_file_category_and_its_view_but_a_lone_file_does_not + +improvise-lpzu, after the user's ruling on PR #314 ("we'd want file identity"). +At `4e68372c`, a folder holding one CSV took the single-file path, because +`import dir/` was exactly `dir/*.csv`. That document had no `File` category and +no per-file view, and the test failed at the `## Category: File` assertion. The +other half pins the direction the ruling did *not* change: the same CSV named +directly imports without any `File`. The rival that merges every all-CSV +import, lone files included, fails that half ("a lone file keeps today's +shape"). So the test separates "a folder merges" from "everything merges". diff --git a/src/main.rs b/src/main.rs index cdd3b83f..a72c7906 100644 --- a/src/main.rs +++ b/src/main.rs @@ -348,8 +348,12 @@ fn get_import_data(args: &[PathBuf]) -> Option { }; let paths = paths.as_slice(); let all_csv = paths.iter().all(|p| csv_path_p(p)); + // A folder always merges, so even one CSV keeps its file identity (lpzu). + let a_folder = classified + .iter() + .any(|a| matches!(a, ImportPath::Folder(_))); - if paths.len() > 1 { + if paths.len() > 1 || a_folder { if !all_csv { eprintln!("Multi-file import only supports CSV files"); return None; diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index b1f33267..fef16fc8 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -708,3 +708,34 @@ fn a_headless_import_takes_files_and_folders_together_and_refuses_an_empty_folde ); assert!(!scratch.path("never.improv").exists()); } + +/// improvise-lpzu, the user's ruling: a folder always merges, so even one CSV +/// keeps its file identity. Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_folder_of_one_csv_still_gets_a_file_category_and_its_view_but_a_lone_file_does_not() { + let scratch = Scratch::new("folder_single"); + let dir = scratch.path("one"); + std::fs::create_dir_all(&dir).unwrap(); + let csv = dir.join("age.csv"); + std::fs::write(&csv, "entity,age-months\nsrc/a.rs,2\n").unwrap(); + + let from_folder = scratch.path("folder.improv"); + let out = run_import(&dir, &from_folder, &[]); + assert!(out.status.success(), "{}", stderr_of(&out)); + let doc = std::fs::read_to_string(&from_folder).unwrap(); + assert!(doc.contains("## Category: File\n- age\n"), "{doc}"); + assert!(doc.lines().any(|l| l == "File: filter, age"), "{doc}"); + assert!( + doc.contains("File=age, _Measure=age-months, entity=|src/a.rs| = 2"), + "{doc}" + ); + + let from_file = scratch.path("file.improv"); + let out = run_import(&csv, &from_file, &[]); + assert!(out.status.success(), "{}", stderr_of(&out)); + let doc = std::fs::read_to_string(&from_file).unwrap(); + assert!( + !doc.contains("File"), + "a lone file keeps today's shape: {doc}" + ); +} From c43ee72402f4c51da15184ec1090866e33f3a60d Mon Sep 17 00:00:00 2001 From: "Claude (agent-lpzu)" Date: Thu, 24 Sep 2026 21:47:11 -0700 Subject: [PATCH 6/6] fix(import): a header-only or measureless file keeps its identity and a view of its own (improvise-lpzu, improvise-8jk6.13, improvise-8jk6.12) ImportPipeline::from_merge seeds the File items and every file's columns from Merged::sources, so a header-only CSV keeps its File item and its columns (unseen columns are proposed as Labels, as an all-null column is). A file with no measure column gets a records view filtered to it instead of paging onto another file's hidden measure. Four per-file-view rules the review found untested now each have a test that fails when the rule is broken. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/formula-and-io.md | 44 ++++- context/test-evidence/cli.md | 4 +- context/test-evidence/formula-and-io.md | 57 ++++++ crates/improvise-io/src/import/csv_parser.rs | 16 ++ crates/improvise-io/src/import/wizard.rs | 173 ++++++++++++++++++- src/main.rs | 4 +- tests/folder_import_views.rs | 14 +- 7 files changed, 295 insertions(+), 17 deletions(-) diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index db85de87..7dd1c322 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -691,8 +691,41 @@ so `Plan::place` gives it no coordinate there (improvise-8jk6.2 above), and no measure cell. Nothing is fabricated: no `0` and no `""`. The analyzer already classified each column over the values present (improvise-l1f5), so a column only one file has is judged by that file's values alone. The category cap -counts the **union** of every file's categories. Rows from different files -cannot collide, because `File` differs. +counts the **union** of every file's categories, header-only files included +(see the next paragraph). Rows from different files cannot collide, because +`File` differs. One caveat on "judged by its values": a JSON `""` is not yet +dropped by the analyzer the way `null` is, so a JSON numeric column holding +`""` is proposed as a Label (improvise-8jk6.11). CSV is unaffected, because +its parser turns a blank field into `null`. + +**A header-only file is still a file** (the Codex P2 on PR #314). code-maat +writes a header and no rows whenever nothing passes its threshold, so +`coupling.csv` is often exactly that. The pipeline is analyzed from *records*, +and such a file has none, so it had no `File` item, its columns never reached +the model, and its view filtered to an item that did not exist. +`ImportPipeline::from_merge` therefore **seeds from `Merged::sources`**: + +- the `File` proposal's items are the sources' items, in file order, so every + file has its item whether it has rows or not; +- a source column that no record has a value in is proposed as a **Label**. + That is the analyzer's own verdict for an all-null column (improvise-l1f5), + and a column with no values is exactly that: nothing can be classified, so it + takes no pivot slot and does not count toward the category cap until the user + reclassifies it in the wizard or with `--category`/`--measure`. + +This is also the file identity the user wants kept for a future update-import. + +**A file with no measure column gets a records view** (improvise-8jk6.13). A +record with no measure makes no cell, so such a file contributes items and no +data. Its view had nothing to page `_Measure` to, and paged onto another file's +measure, which the same view hides; the cell there was a `Record`, editable, +under this file's `File`. Now `source_view` gives such a file the records +layout (`view::records_steps`, the one home of what a records view is) with +`File` still on Filter at the file's item. It draws zero rows, because there +are no cells. A header-only file whose columns are +all Labels is the same case. In records mode the columns are the whole model's +categories and measures, not just this file's: records mode has no per-view +column filter. With no rows that is cosmetic. **Each file gets a view** (the user, 2026-09-25: "dir import should do this: one view filtered by the file column and projected to the columns in that @@ -721,7 +754,12 @@ initial view. Each per-file view: records lack those, so nothing splits. - **Puts `_Measure` on Page**, as `Workbook::new` does, **with other files' measures hidden** and the page selection on the file's first measure. A - formula target comes from no file, so it stays visible. + formula target comes from no file, so it stays visible. A file with no measure + gets a records view instead (above). +- **Is never named like another view from the same import.** `Default.csv` and + `Default 2.csv` become `Default 2` and `Default 2 2`; without the same-batch + check both would be named `Default 2` and one view would silently replace the + other. A per-file view is made only while `File` is an accepted category. A user who skipped or reclassified it has no filter to build views from. diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 1145ee78..96ee4f9e 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -444,7 +444,9 @@ or browser test was written. For each file's view: - the drawn cells are that file's alone: `2.00, 7.00` for age and `5.00, 3.00` for coupling. -Rivals R2, R7 and R9 each fail it. +Rivals R2, R7 and R9 each fail it. Since the review of `4e68372c` the fixture +also holds a header-only `soc.csv`. Its view must carry `File` at `soc`, be a +records view, and draw zero rows. Rivals F7 and F8 fail that half. ### a_folder_of_one_csv_still_gets_a_file_category_and_its_view_but_a_lone_file_does_not diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 71710e03..887302e6 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1702,6 +1702,14 @@ improvise-lpzu. `[z.csv, b/, a.csv]` must expand to is sorted within itself. A fixture listing only folders, or files in name order, could not tell "argument order" from "sort everything". R6 fails it. +### two_files_with_one_stem_union_their_columns + +improvise-8jk6.12 (3). `a/x.csv` (`entity,one`) and `b/x.csv` (`entity,two`) are +one `File` item, `x`, whose columns are `entity, one, two`. Rival F3 (the +reviewer's fault: clear the stem's columns before each file) leaves +`entity, two`, and the `x` view would then hide the measure `one` that half its +records carry. + ### files_with_different_headers_merge_without_inventing_the_missing_columns improvise-lpzu, at the parser. Each merged record carries exactly its own @@ -1767,3 +1775,52 @@ identical `.improv` text, and `format(parse(text)) == text`. The reloaded document still has the three views in order, `File: filter` at the file's item, and the same drawn values. This is the first persisted, non-virtual use of `Axis::Filter` (gotcha 27). R7 and R9 fail it. + +### a_header_only_file_keeps_its_file_item_its_columns_and_its_view + +The ChatGPT Codex P2 on PR #314 at `4e68372c`, confirmed by the adversarial +review. `age.csv` (one row), a header-only `coupling.csv` and `rev.csv` (one +row). At `005b7a76` the test failed at the first assertion: `File` held only +`age` and `rev`, so the `coupling` view filtered to an item that did not exist, +and `coupled`, `degree` and `average-revs` were not in the model. Now `File` +reads `age, coupling, rev` in file order, the three columns exist (as Labels), +and the `coupling` view is a zero-row records view filtered to `coupling`, while +the `age` view still draws `2`. Rival F5 (sources not seeded) and F6 (unseen +columns not proposed) each fail it, and so do F7 and F8 below. + +### a_file_with_no_measure_gets_a_records_view_not_another_files_measure + +improvise-8jk6.13, from the review of PR #314. `a.csv` (`entity,x`) and +`tags.csv` (`entity,tag`). At `005b7a76` the `tags` view had `_Measure` on Page +with no selection, so the layout paged onto `x`, a measure the same view hides. +The test failed there, with `row_count() == 1`: `a`'s record, drawn under +`File=tags`'s view. Now the view has no `_Measure` page, is a records layout, +and draws zero rows, while `a`'s view is still a pivot. Rival F7 (the old +paging) fails it, and F8 (the File selection lost when `records_steps` moves +`File` off Filter) fails it with the same `1`. F8 is the mistake the first +version of the fix made. + +### a_files_view_seats_the_date_parts_of_its_own_date_column + +improvise-8jk6.12 (1). The reviewer replaced the date-parts clause of +`source_view`'s `own` with `false` and every test passed, because no folder +fixture extracted a date part. Here `abs-churn.csv` has a `date` column, +extracted to `date_Month`, and `age.csv` does not. `date_Month` is seated in the +`abs-churn` view and on None in the `age` view. Rival F1 (the reviewer's fault) +puts it on None in both and fails the first assertion. + +### two_files_whose_names_collide_in_one_batch_keep_a_view_each + +improvise-8jk6.12 (2). `Default.csv` and `Default 2.csv`. The first takes +`Default 2`, so the second must take `Default 2 2`. Without the same-batch +check both are named `Default 2`, and `IndexMap::insert` silently replaces the +first, leaving two views where three are owed. The test asserts all three names +with their filters. Rival F2 (the reviewer's fault) fails it. + +### a_whitespace_only_category_value_is_no_coordinate + +improvise-8jk6.12 (4). A `region` of `" "` is kept as a record with no +`region` coordinate, and `region` holds only `East`. The trim in `scalar_name` +predates this PR (improvise-ych), but the sparse-record rule now rests on it: +without it the value becomes an item named `" "`. Rival F4 (the reviewer's +fault) fails it. diff --git a/crates/improvise-io/src/import/csv_parser.rs b/crates/improvise-io/src/import/csv_parser.rs index 6f2120de..355b27fb 100644 --- a/crates/improvise-io/src/import/csv_parser.rs +++ b/crates/improvise-io/src/import/csv_parser.rs @@ -500,6 +500,22 @@ mod tests { ); } + /// improvise-8jk6.12 (3): two files with one stem are one item, and its + /// columns are both files'. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn two_files_with_one_stem_union_their_columns() { + let fs = folder(&[ + ("a/x.csv", "entity,one\ne,1\n"), + ("b/x.csv", "entity,two\ne,2\n"), + ]); + let merged = merge_csvs_from(&fs, &["a/x.csv", "b/x.csv"]).unwrap(); + let expected = SourceFile { + item: "x".to_string(), + columns: ["entity", "one", "two"].map(String::from).to_vec(), + }; + assert_eq!(merged.sources, [expected]); + } + // ── RFC 4180 edge cases ─────────────────────────────────────────── #[test] diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index 232e5e19..7a4e2ab2 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -7,11 +7,11 @@ use super::analyzer::{ DateComponent, FieldKind, FieldProposal, analyze_records, extract_array_at_path, extract_date_component, find_array_paths, }; -use super::csv_parser::{FILE_CATEGORY, SourceFile}; +use super::csv_parser::{FILE_CATEGORY, Merged, SourceFile}; use crate::formula::parse_formula; use crate::model::MAX_CATEGORIES; use crate::model::cell::{CellKey, CellValue}; -use crate::view::{Axis, VIRTUAL_VIEW_NAMES, View}; +use crate::view::{Axis, VIRTUAL_VIEW_NAMES, View, ViewStep, records_steps}; use crate::workbook::Workbook; // ── Pipeline (no UI state) ──────────────────────────────────────────────────── @@ -82,6 +82,32 @@ impl ImportPipeline { pipeline } + /// A pipeline over a CSV merge, carrying its sources for the per-file views. + pub fn from_merge(merged: Merged) -> Self { + let mut pipeline = Self::new(Value::Array(merged.records)); + pipeline.seed_sources(merged.sources); + pipeline + } + + /// Every file keeps its `File` item and its columns, rows or none: a + /// header-only file is still a file (improvise-lpzu). + fn seed_sources(&mut self, sources: Vec) { + let items: Vec = sources.iter().map(|s| s.item.clone()).collect(); + match self.proposals.iter_mut().find(|p| p.field == FILE_CATEGORY) { + Some(file) => file.distinct_values = items, + None => self + .proposals + .push(unseen(FILE_CATEGORY, FieldKind::Category, items)), + } + for column in sources.iter().flat_map(|s| &s.columns) { + if !self.proposals.iter().any(|p| &p.field == column) { + self.proposals + .push(unseen(column, FieldKind::Label, vec![])); + } + } + self.sources = sources; + } + pub fn select_path(&mut self, path: &str) { self.selected_path = path.to_string(); if let Some(arr) = extract_array_at_path(&self.raw, path) { @@ -350,6 +376,19 @@ impl ImportPipeline { } } +/// A proposal for a column no record has a value in, which is what an +/// all-null column already gets from the analyzer (improvise-l1f5). +fn unseen(field: &str, kind: FieldKind, distinct_values: Vec) -> FieldProposal { + FieldProposal { + field: field.to_string(), + kind, + distinct_values, + accepted: true, + date_format: None, + date_components: vec![], + } +} + /// One view per merged file, in file order, while `File` is a category. The /// shape and why Filter: `context/code/formula-and-io.md` (improvise-lpzu). fn source_views(wb: &Workbook, sources: &[SourceFile], plan: &Plan) -> Vec { @@ -398,12 +437,26 @@ fn source_view(wb: &Workbook, source: &SourceFile, plan: &Plan, name: String) -> for measure in theirs { view.hide_item("_Measure", &measure.field); } - if let Some(first) = mine.first() { - view.set_page_selection("_Measure", &first.field); + match mine.first() { + Some(first) => view.set_page_selection("_Measure", &first.field), + None => into_records(&mut view, wb, &source.item), } view } +/// A file with no measure has no measure to page to: its view is its records, +/// still filtered to the file (improvise-8jk6.13). +fn into_records(view: &mut View, wb: &Workbook, item: &str) { + for step in records_steps(&wb.model) { + match step { + ViewStep::Axis { category, axis } => view.set_axis(&category, axis), + ViewStep::PageSelection { category, item } => view.set_page_selection(&category, &item), + } + } + view.set_axis(FILE_CATEGORY, Axis::Filter); + view.set_page_selection(FILE_CATEGORY, item); +} + /// The accepted fields by role, and the date parts derived from them. struct Plan<'a> { categories: Vec<&'a FieldProposal>, @@ -2176,9 +2229,7 @@ mod tests { let paths: Vec<&str> = files.iter().map(|(p, _)| *p).collect(); let merged = merge_csvs_from(&fs, &paths).unwrap(); assert!(merged.warnings.is_empty(), "{:?}", merged.warnings); - let mut p = ImportPipeline::new(Value::Array(merged.records)); - p.sources = merged.sources; - p + ImportPipeline::from_merge(merged) } /// code-maat's age and coupling reports. @@ -2263,6 +2314,114 @@ mod tests { assert_eq!((drill("Payee", "Bolt"), drill("Date", "d1")), (1, 1)); } + /// improvise-lpzu, the Codex P2 on PR #314: a header-only file keeps its + /// identity. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_header_only_file_keeps_its_file_item_its_columns_and_its_view() { + use crate::view::GridLayout; + let (wb, _) = merged_pipeline(&[ + ("maat/age.csv", "entity,age-months\nsrc/a.rs,2\n"), + ("maat/coupling.csv", "entity,coupled,degree,average-revs\n"), + ("maat/rev.csv", "entity,n-revs\nsrc/a.rs,9\n"), + ]) + .build_model_with_warnings() + .unwrap(); + let files: Vec<&String> = wb.model.category("File").unwrap().items.keys().collect(); + assert_eq!(files, ["age", "coupling", "rev"]); + for column in ["coupled", "degree", "average-revs"] { + assert!(wb.model.category(column).is_some(), "{column} is missing"); + } + let view = &wb.views["coupling"]; + assert_eq!(view.page_selection("File"), Some("coupling")); + let layout = GridLayout::with_frozen_records(&wb.model, view, None); + assert!(layout.is_records_mode() && layout.row_count() == 0); + assert_eq!(drawn(&wb, "age"), [2.0]); + } + + /// improvise-8jk6.13: a view never pages onto a measure it hides. + /// Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_file_with_no_measure_gets_a_records_view_not_another_files_measure() { + use crate::view::GridLayout; + let (wb, _) = merged_pipeline(&[ + ("t/a.csv", "entity,x\ne1,1\n"), + ("t/tags.csv", "entity,tag\ne1,red\n"), + ]) + .build_model_with_warnings() + .unwrap(); + let tags = &wb.views["tags"]; + assert_eq!(tags.page_selection("_Measure"), None); + assert_ne!(tags.axis_of("_Measure"), Axis::Page, "{tags:?}"); + let layout = GridLayout::with_frozen_records(&wb.model, tags, None); + assert!(layout.is_records_mode(), "{tags:?}"); + assert_eq!( + layout.row_count(), + 0, + "a record with no measure has no cell" + ); + assert!(!wb.views["a"].is_records_layout()); + } + + /// improvise-8jk6.12 (1): a date part derived from the file's own column + /// is the file's. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_files_view_seats_the_date_parts_of_its_own_date_column() { + use crate::import::analyzer::DateComponent; + let mut p = merged_pipeline(&[ + ("m/abs-churn.csv", "date,added\n2026-01-02,10\n"), + ("m/age.csv", "entity,age-months\nsrc/a.rs,2\n"), + ]); + let date = p.proposals.iter_mut().find(|f| f.field == "date").unwrap(); + assert_eq!(date.kind, FieldKind::TimeCategory); + date.date_components = vec![DateComponent::Month]; + let (wb, _) = p.build_model_with_warnings().unwrap(); + assert_ne!(wb.views["abs-churn"].axis_of("date_Month"), Axis::None); + assert_eq!(wb.views["age"].axis_of("date_Month"), Axis::None); + } + + /// improvise-8jk6.12 (2): two files of one batch never share a view name. + /// Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn two_files_whose_names_collide_in_one_batch_keep_a_view_each() { + let (wb, _) = merged_pipeline(&[ + ("d/Default.csv", "Region,Revenue\nEast,1\n"), + ("d/Default 2.csv", "Region,Revenue\nEast,2\n"), + ]) + .build_model_with_warnings() + .unwrap(); + let filters: Vec<(&str, Option<&str>)> = wb + .views + .values() + .map(|v| (v.name.as_str(), v.page_selection("File"))) + .collect(); + assert_eq!( + filters, + [ + ("Default", None), + ("Default 2", Some("Default")), + ("Default 2 2", Some("Default 2")), + ] + ); + } + + /// improvise-8jk6.12 (4): a whitespace-only value is no coordinate, never + /// an item. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_whitespace_only_category_value_is_no_coordinate() { + let (wb, _) = ImportPipeline::new(json!([ + {"region": "East", "revenue": 1.0}, + {"region": " ", "revenue": 2.0}, + ])) + .build_model_with_warnings() + .unwrap(); + let items: Vec<&String> = wb.model.category("region").unwrap().items.keys().collect(); + assert_eq!(items, ["East"]); + assert_eq!( + stored(&wb), + ["_Measure=revenue, region=East = 1", "_Measure=revenue = 2"] + ); + } + /// improvise-lpzu: each merged file gets a view of its own records and /// columns. Evidence: `context/test-evidence/formula-and-io.md`. #[test] diff --git a/src/main.rs b/src/main.rs index a72c7906..fa0dbc67 100644 --- a/src/main.rs +++ b/src/main.rs @@ -364,9 +364,7 @@ fn get_import_data(args: &[PathBuf]) -> Option { for warning in &merged.warnings { eprintln!("warning: {warning}"); } - let mut pipeline = ImportPipeline::new(Value::Array(merged.records)); - pipeline.sources = merged.sources; - Some(pipeline) + Some(ImportPipeline::from_merge(merged)) } Err(e) => { eprintln!("CSV merge error: {e:#}"); diff --git a/tests/folder_import_views.rs b/tests/folder_import_views.rs index 43a8d0f4..0c26289b 100644 --- a/tests/folder_import_views.rs +++ b/tests/folder_import_views.rs @@ -19,15 +19,16 @@ fn reloaded_folder_import() -> App { "maat/coupling.csv", "entity,coupled,degree,average-revs\nsrc/a.rs,src/b.rs,80,5\nsrc/b.rs,src/c.rs,40,3\n", ), + ("maat/soc.csv", "entity,soc\n"), ]; for (path, body) in files { fs.write(std::path::Path::new(path), body.as_bytes()) .unwrap(); } let merged = merge_csvs_from(&fs, &files.map(|(p, _)| p)).unwrap(); - let mut pipeline = ImportPipeline::new(serde_json::Value::Array(merged.records)); - pipeline.sources = merged.sources; - let (wb, _) = pipeline.build_model_with_warnings().unwrap(); + let (wb, _) = ImportPipeline::from_merge(merged) + .build_model_with_warnings() + .unwrap(); App::new(parse_md(&format_md(&wb)).unwrap(), None) } @@ -54,4 +55,11 @@ fn a_saved_per_file_view_projects_only_its_file() { .collect(); assert_eq!(drawn, texts, "{view}"); } + + // A header-only file: its item exists, and its view has nothing editable. + let soc = grid_in(&app, "soc", whole); + let file = soc.page_coords.iter().find(|p| p.cat == "File").unwrap(); + assert_eq!(file.selected, "soc"); + assert!(soc.records, "a file with no measure opens in records mode"); + assert_eq!(soc.extent.rows, 0, "{soc:?}"); }