fix(import): headless import refuses instead of silently succeeding (improvise-8jk6.4, improvise-8jk6.5, improvise-8jk6.6)

- 8jk6.4: the CSV parser yields source text (CsvRecords), and only the
  role reads it as a number. A forced category keeps 00565796 and
  220944e2 verbatim; measures and analysis read the text through
  csv_parser::number_in. CsvRecords reaches a pipeline only through
  ImportPipeline::from_csv, so handing CSV text to the JSON constructor
  no longer compiles.
- 8jk6.5: an import formula that does not parse is refused by name with
  its parse error: the build refuses (headless exits non-zero and writes
  nothing), and the wizard keeps it out of the list with the text still
  in the buffer.
- 8jk6.6: an import that kept no record refuses and says why, from the
  build and from the wizard's review step. A partial skip stays a
  warning with a count and exit 0.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Claude (agent-headless-import)
2026-09-24 21:54:39 -07:00
committed by Edward Langley
parent 4e68372c25
commit 8bd4b1a77b
9 changed files with 439 additions and 112 deletions
+21 -32
View File
@@ -1239,7 +1239,7 @@ pub struct ImportJsonHeadless {
} }
impl Effect for ImportJsonHeadless { impl Effect for ImportJsonHeadless {
fn apply(&self, app: &mut App) { fn apply(&self, app: &mut App) {
use crate::import::analyzer::{FieldKind, analyze_records}; use crate::import::analyzer::FieldKind;
use crate::import::wizard::{DuplicatePolicy, ImportPipeline}; use crate::import::wizard::{DuplicatePolicy, ImportPipeline};
let is_csv = self let is_csv = self
@@ -1247,10 +1247,10 @@ impl Effect for ImportJsonHeadless {
.extension() .extension()
.is_some_and(|ext| ext.eq_ignore_ascii_case("csv")); .is_some_and(|ext| ext.eq_ignore_ascii_case("csv"));
// Parse the file exactly once: `records` feeds analysis, `raw` is reused // Parse the file exactly once, into the pipeline that reads it
// as the pipeline's value (improvise-oaq). CSV warnings go to the status. // (improvise-oaq, improvise-8jk6.4). CSV warnings go to the status.
let mut warnings: Vec<String> = vec![]; let mut warnings: Vec<String> = vec![];
let (records, raw) = if is_csv { let mut pipeline = if is_csv {
let bytes = match app.storage.read(&self.path) { let bytes = match app.storage.read(&self.path) {
Ok(b) => b, Ok(b) => b,
Err(e) => { Err(e) => {
@@ -1261,8 +1261,7 @@ impl Effect for ImportJsonHeadless {
match crate::import::csv_parser::parse_csv_bytes_with_warnings(&bytes) { match crate::import::csv_parser::parse_csv_bytes_with_warnings(&bytes) {
Ok((recs, w)) => { Ok((recs, w)) => {
warnings = w; warnings = w;
let raw = serde_json::Value::Array(recs.clone()); ImportPipeline::from_csv(recs)
(recs, raw)
} }
Err(e) => { Err(e) => {
refuse_and_stop(app, format!("CSV error: {e}")); refuse_and_stop(app, format!("CSV error: {e}"));
@@ -1285,38 +1284,28 @@ impl Effect for ImportJsonHeadless {
} }
}; };
match json_import_records(&value, self.array_path.as_deref()) { match json_import_records(&value, self.array_path.as_deref()) {
Ok(recs) => (recs, value), Ok(recs) => {
let mut pipeline = ImportPipeline::new(value);
pipeline.selected_path = self.array_path.clone().unwrap_or_default();
pipeline.records = recs;
pipeline.analyze();
pipeline
}
Err(msg) => { Err(msg) => {
refuse_and_stop(app, msg); refuse_and_stop(app, msg);
return; return;
} }
} }
}; };
pipeline.model_name = self
let proposals = analyze_records(&records); .model_name
.clone()
let pipeline = ImportPipeline { .unwrap_or_else(|| "Imported Model".to_string());
raw, // Headless: nobody to ask, so a collision refuses (improvise-cgqt).
array_paths: vec![], pipeline.on_duplicate = DuplicatePolicy::Refuse;
selected_path: self.array_path.as_deref().unwrap_or("").to_string(), for p in &mut pipeline.proposals {
records, p.accepted = p.kind != FieldKind::Label;
proposals: proposals }
.into_iter()
.map(|mut p| {
p.accepted = p.kind != FieldKind::Label;
p
})
.collect(),
model_name: self
.model_name
.as_deref()
.unwrap_or("Imported Model")
.to_string(),
formulas: vec![],
// Headless: nobody to ask, so a collision refuses (improvise-cgqt).
on_duplicate: DuplicatePolicy::Refuse,
sources: vec![],
};
match pipeline.build_model_with_warnings() { match pipeline.build_model_with_warnings() {
Ok((new_workbook, build_warnings)) => { Ok((new_workbook, build_warnings)) => {
+73 -50
View File
@@ -7,14 +7,32 @@ use serde_json::Value;
use crate::storage::{PhysicalFs, Storage}; use crate::storage::{PhysicalFs, Storage};
/// Parsed CSV records. Every value is its source text, so they reach an import
/// only through `ImportPipeline::from_csv` (improvise-8jk6.4).
#[derive(Debug, Clone, Default, PartialEq)]
pub struct CsvRecords(Vec<Value>);
impl std::ops::Deref for CsvRecords {
type Target = [Value];
fn deref(&self) -> &[Value] {
&self.0
}
}
impl CsvRecords {
pub(crate) fn into_values(self) -> Vec<Value> {
self.0
}
}
pub fn csv_path_p(path: &Path) -> bool { pub fn csv_path_p(path: &Path) -> bool {
path.extension() path.extension()
.is_some_and(|ext| ext.eq_ignore_ascii_case("csv")) .is_some_and(|ext| ext.eq_ignore_ascii_case("csv"))
} }
/// Parse a CSV file into a `serde_json::Value` array. Short-row warnings go to /// Parse a CSV file into records. Short-row warnings go to stderr;
/// stderr; [`parse_csv_with_warnings`] captures them instead. /// [`parse_csv_with_warnings`] captures them instead.
pub fn parse_csv(path: &Path) -> Result<Vec<Value>> { pub fn parse_csv(path: &Path) -> Result<CsvRecords> {
let (records, warnings) = parse_csv_with_warnings(path)?; let (records, warnings) = parse_csv_with_warnings(path)?;
for warning in &warnings { for warning in &warnings {
eprintln!("warning: {warning}"); eprintln!("warning: {warning}");
@@ -24,7 +42,7 @@ pub fn parse_csv(path: &Path) -> Result<Vec<Value>> {
/// Parse a CSV file, returning records plus warnings. A short row keeps every /// Parse a CSV file, returning records plus warnings. A short row keeps every
/// header column, missing ones `Null`, each reported with a row count. /// header column, missing ones `Null`, each reported with a row count.
pub fn parse_csv_with_warnings(path: &Path) -> Result<(Vec<Value>, Vec<String>)> { pub fn parse_csv_with_warnings(path: &Path) -> Result<(CsvRecords, Vec<String>)> {
let bytes = std::fs::read(path) let bytes = std::fs::read(path)
.with_context(|| format!("Failed to open CSV file: {}", path.display()))?; .with_context(|| format!("Failed to open CSV file: {}", path.display()))?;
parse_csv_bytes_with_warnings(&bytes) parse_csv_bytes_with_warnings(&bytes)
@@ -32,9 +50,9 @@ pub fn parse_csv_with_warnings(path: &Path) -> Result<(Vec<Value>, Vec<String>)>
/// As [`parse_csv_with_warnings`] but over bytes, so import can come from any /// 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). /// [`Storage`](crate::storage::Storage) backend, not only a file (improvise-fuj).
pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(Vec<Value>, Vec<String>)> { pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(CsvRecords, Vec<String>)> {
let (_, records, warnings) = parse_csv_bytes(bytes)?; let (_, records, warnings) = parse_csv_bytes(bytes)?;
Ok((records, warnings)) Ok((CsvRecords(records), warnings))
} }
/// The header (after renaming), the records and the warnings. /// The header (after renaming), the records and the warnings.
@@ -152,7 +170,7 @@ fn disambiguate_headers(raw: Vec<String>) -> (Vec<String>, Vec<String>) {
/// Parse multiple CSV files and merge into a single JSON array. /// 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"). /// Each record gets a "File" field set to the filename stem (e.g., "sales" from "sales.csv").
pub fn merge_csvs(paths: &[impl AsRef<Path>]) -> Result<Vec<Value>> { pub fn merge_csvs(paths: &[impl AsRef<Path>]) -> Result<CsvRecords> {
let merged = merge_csvs_from(&PhysicalFs, paths)?; let merged = merge_csvs_from(&PhysicalFs, paths)?;
for warning in &merged.warnings { for warning in &merged.warnings {
eprintln!("warning: {warning}"); eprintln!("warning: {warning}");
@@ -173,7 +191,7 @@ pub struct SourceFile {
/// Several CSV files as one record set, with each file's own columns kept. /// Several CSV files as one record set, with each file's own columns kept.
#[derive(Debug, Clone, PartialEq)] #[derive(Debug, Clone, PartialEq)]
pub struct Merged { pub struct Merged {
pub records: Vec<Value>, pub records: CsvRecords,
/// One per distinct `File` item, in the order the files were read. /// One per distinct `File` item, in the order the files were read.
pub sources: Vec<SourceFile>, pub sources: Vec<SourceFile>,
/// Each file's parse warnings, prefixed with its path. /// Each file's parse warnings, prefixed with its path.
@@ -184,7 +202,7 @@ pub struct Merged {
/// and two files with one stem share its item and its columns (improvise-lpzu). /// 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<Path>]) -> Result<Merged> { pub fn merge_csvs_from(fs: &dyn Storage, paths: &[impl AsRef<Path>]) -> Result<Merged> {
let mut merged = Merged { let mut merged = Merged {
records: Vec::new(), records: CsvRecords::default(),
sources: Vec::new(), sources: Vec::new(),
warnings: Vec::new(), warnings: Vec::new(),
}; };
@@ -212,7 +230,7 @@ pub fn merge_csvs_from(fs: &dyn Storage, paths: &[impl AsRef<Path>]) -> Result<M
if let Value::Object(ref mut map) = record { if let Value::Object(ref mut map) = record {
map.insert(FILE_CATEGORY.to_string(), Value::String(stem.clone())); map.insert(FILE_CATEGORY.to_string(), Value::String(stem.clone()));
} }
merged.records.push(record); merged.records.0.push(record);
} }
} }
Ok(merged) Ok(merged)
@@ -266,26 +284,33 @@ fn csv_documents_in(fs: &dyn Storage, dir: &Path) -> Result<Vec<PathBuf>> {
Ok(csvs) Ok(csvs)
} }
/// A field is its source text: which ones are numbers is decided by the role
/// the import gives the column (improvise-8jk6.4, `context/code/formula-and-io.md`).
fn parse_csv_field(field: &str) -> Value { fn parse_csv_field(field: &str) -> Value {
if field.is_empty() { match field.is_empty() {
return Value::Null; true => Value::Null,
false => Value::String(field.to_string()),
} }
}
// Try to parse as number (integer or float) /// The number a CSV field's text reads as, where a number is wanted.
if let Ok(num) = field.parse::<i64>() { pub fn number_in(text: &str) -> Option<serde_json::Number> {
return Value::Number(serde_json::Number::from(num)); if let Ok(num) = text.parse::<i64>() {
return Some(serde_json::Number::from(num));
} }
// NaN and the infinities have no JSON number. A fabricated 0 is an invented // NaN and the infinities have no JSON number. A fabricated 0 is an invented
// measurement (improvise-jc95), so the token falls through and stays text. // measurement (improvise-jc95), so the token stays text.
if let Ok(num) = field.parse::<f64>() text.parse::<f64>()
&& let Some(number) = serde_json::Number::from_f64(num) .ok()
{ .and_then(serde_json::Number::from_f64)
return Value::Number(number); }
}
// Otherwise treat as string /// A CSV value as a typed JSON value: numeric text becomes its number.
Value::String(field.to_string()) pub fn typed(value: &Value) -> Value {
match value.as_str().and_then(number_in) {
Some(number) => Value::Number(number),
None => value.clone(),
}
} }
#[cfg(test)] #[cfg(test)]
@@ -310,8 +335,9 @@ mod tests {
assert_eq!(records.len(), 2); assert_eq!(records.len(), 2);
assert_eq!(records[0]["Region"], Value::String("East".to_string())); assert_eq!(records[0]["Region"], Value::String("East".to_string()));
assert_eq!(records[0]["Product"], Value::String("Shirts".to_string())); assert_eq!(records[0]["Product"], Value::String("Shirts".to_string()));
assert_eq!(records[0]["Revenue"], Value::String("1000".to_string()));
assert_eq!( assert_eq!(
records[0]["Revenue"], typed(&records[0]["Revenue"]),
Value::Number(serde_json::Number::from(1000)) Value::Number(serde_json::Number::from(1000))
); );
} }
@@ -323,9 +349,10 @@ mod tests {
let records = parse_csv(&path).unwrap(); let records = parse_csv(&path).unwrap();
assert_eq!(records.len(), 2); assert_eq!(records.len(), 2);
assert!(records[0]["Revenue"].is_f64()); assert_eq!(records[0]["Revenue"], Value::String("1000.50".to_string()));
assert!(typed(&records[0]["Revenue"]).is_f64());
assert_eq!( assert_eq!(
records[0]["Revenue"], typed(&records[0]["Revenue"]),
Value::Number(serde_json::Number::from_f64(1000.50).unwrap()) Value::Number(serde_json::Number::from_f64(1000.50).unwrap())
); );
} }
@@ -363,10 +390,10 @@ mod tests {
assert_eq!(records.len(), 2); assert_eq!(records.len(), 2);
assert_eq!(records[0]["Name"], Value::String("Widget".to_string())); assert_eq!(records[0]["Name"], Value::String("Widget".to_string()));
assert_eq!( assert_eq!(
records[0]["Count"], typed(&records[0]["Count"]),
Value::Number(serde_json::Number::from(5)) Value::Number(serde_json::Number::from(5))
); );
assert!(records[0]["Price"].is_f64()); assert!(typed(&records[0]["Price"]).is_f64());
assert_eq!(records[0]["Active"], Value::String("true".to_string())); assert_eq!(records[0]["Active"], Value::String("true".to_string()));
} }
@@ -386,10 +413,7 @@ mod tests {
assert_eq!(records[3]["File"], Value::String("expenses".to_string())); assert_eq!(records[3]["File"], Value::String("expenses".to_string()));
// Original fields preserved // Original fields preserved
assert_eq!(records[0]["Region"], Value::String("East".to_string())); assert_eq!(records[0]["Region"], Value::String("East".to_string()));
assert_eq!( assert_eq!(records[2]["Revenue"], Value::String("50".to_string()));
records[2]["Revenue"],
Value::Number(serde_json::Number::from(50))
);
} }
#[test] #[test]
@@ -492,10 +516,10 @@ mod tests {
] ]
); );
assert_eq!( assert_eq!(
merged.records, *merged.records,
[ [
serde_json::json!({"entity": "src/a.rs", "coupled": "src/b.rs", "degree": 80, "File": "coupling"}), 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"}), serde_json::json!({"entity": "src/a.rs", "age-months": "2", "File": "age"}),
] ]
); );
} }
@@ -640,10 +664,7 @@ mod tests {
assert!(warnings.is_empty()); assert!(warnings.is_empty());
assert_eq!(records.len(), 2); assert_eq!(records.len(), 2);
assert_eq!(records[0]["Region"], Value::String("East".to_string())); assert_eq!(records[0]["Region"], Value::String("East".to_string()));
assert_eq!( assert_eq!(records[0]["Revenue"], Value::String("1".to_string()));
records[0]["Revenue"],
Value::Number(serde_json::Number::from(1))
);
assert_eq!(records[1]["Region"], Value::String("West".to_string())); assert_eq!(records[1]["Region"], Value::String("West".to_string()));
let (control, _) = parse_csv_bytes_with_warnings(b"Region,Revenue\nEa\rst,1\n").unwrap(); let (control, _) = parse_csv_bytes_with_warnings(b"Region,Revenue\nEa\rst,1\n").unwrap();
@@ -698,7 +719,8 @@ mod tests {
token.parse::<f64>().is_ok_and(|n| !n.is_finite()), token.parse::<f64>().is_ok_and(|n| !n.is_finite()),
"precondition: {token} parses as a non-finite f64" "precondition: {token} parses as a non-finite f64"
); );
assert_eq!(parse_csv_field(token), Value::String(token.to_string())); let text = Value::String(token.to_string());
assert_eq!(typed(&text), text);
} }
} }
@@ -707,10 +729,10 @@ mod tests {
#[test] #[test]
fn an_ordinary_float_is_still_a_number() { fn an_ordinary_float_is_still_a_number() {
assert_eq!( assert_eq!(
parse_csv_field("1000.50"), typed(&Value::String("1000.50".into())),
Value::Number(serde_json::Number::from_f64(1000.50).unwrap()) Value::Number(serde_json::Number::from_f64(1000.50).unwrap())
); );
assert_eq!(parse_csv_field("0"), Value::Number(0.into())); assert_eq!(typed(&Value::String("0".into())), Value::Number(0.into()));
} }
/// improvise-5hfh: two equal header names are one JSON key, so the later /// improvise-5hfh: two equal header names are one JSON key, so the later
@@ -719,9 +741,9 @@ mod tests {
fn a_repeated_header_is_renamed_rather_than_overwriting_its_twin() { fn a_repeated_header_is_renamed_rather_than_overwriting_its_twin() {
let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B,A\n1,2,3\n").unwrap(); let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B,A\n1,2,3\n").unwrap();
let map = records[0].as_object().unwrap(); let map = records[0].as_object().unwrap();
assert_eq!(map["A"], Value::Number(1.into())); assert_eq!(map["A"], Value::String("1".into()));
assert_eq!(map["B"], Value::Number(2.into())); assert_eq!(map["B"], Value::String("2".into()));
assert_eq!(map["A_2"], Value::Number(3.into())); assert_eq!(map["A_2"], Value::String("3".into()));
assert_eq!(warnings.len(), 1); assert_eq!(warnings.len(), 1);
assert!(warnings[0].contains("'A'") && warnings[0].contains("'A_2'")); assert!(warnings[0].contains("'A'") && warnings[0].contains("'A_2'"));
assert!(warnings[0].contains("column 3"), "{}", warnings[0]); assert!(warnings[0].contains("column 3"), "{}", warnings[0]);
@@ -734,8 +756,8 @@ mod tests {
let (records, _) = parse_csv_bytes_with_warnings(b"A,A_2,A\n1,2,3\n").unwrap(); let (records, _) = parse_csv_bytes_with_warnings(b"A,A_2,A\n1,2,3\n").unwrap();
let map = records[0].as_object().unwrap(); let map = records[0].as_object().unwrap();
assert_eq!(map.len(), 3, "{map:?}"); assert_eq!(map.len(), 3, "{map:?}");
assert_eq!(map["A_2"], Value::Number(2.into())); assert_eq!(map["A_2"], Value::String("2".into()));
assert_eq!(map["A_3"], Value::Number(3.into())); assert_eq!(map["A_3"], Value::String("3".into()));
} }
#[test] #[test]
@@ -770,8 +792,9 @@ mod tests {
assert_eq!(records.len(), 2); assert_eq!(records.len(), 2);
assert_eq!(records[0]["Date"], Value::String("03/31/2026".to_string())); assert_eq!(records[0]["Date"], Value::String("03/31/2026".to_string()));
assert_eq!(records[0]["Amount"], Value::String("-50.00".to_string()));
assert_eq!( assert_eq!(
records[0]["Amount"], typed(&records[0]["Amount"]),
Value::Number(serde_json::Number::from_f64(-50.00).unwrap()) Value::Number(serde_json::Number::from_f64(-50.00).unwrap())
); );
assert_eq!(records[0]["Flag"], Value::String("*".to_string())); assert_eq!(records[0]["Flag"], Value::String("*".to_string()));
@@ -781,7 +804,7 @@ mod tests {
Value::String("VENMO PAYMENT 260331".to_string()) Value::String("VENMO PAYMENT 260331".to_string())
); );
assert_eq!( assert_eq!(
records[1]["Amount"], typed(&records[1]["Amount"]),
Value::Number(serde_json::Number::from_f64(-240.00).unwrap()) Value::Number(serde_json::Number::from_f64(-240.00).unwrap())
); );
} }
+221 -20
View File
@@ -7,8 +7,8 @@ use super::analyzer::{
DateComponent, FieldKind, FieldProposal, analyze_records, extract_array_at_path, DateComponent, FieldKind, FieldProposal, analyze_records, extract_array_at_path,
extract_date_component, find_array_paths, extract_date_component, find_array_paths,
}; };
use super::csv_parser::{FILE_CATEGORY, SourceFile}; use super::csv_parser::{self, CsvRecords, FILE_CATEGORY, SourceFile};
use crate::formula::parse_formula; use crate::formula::{Formula, parse_formula};
use crate::model::MAX_CATEGORIES; use crate::model::MAX_CATEGORIES;
use crate::model::cell::{CellKey, CellValue}; use crate::model::cell::{CellKey, CellValue};
use crate::view::{Axis, VIRTUAL_VIEW_NAMES, View}; use crate::view::{Axis, VIRTUAL_VIEW_NAMES, View};
@@ -33,6 +33,42 @@ pub struct ImportPipeline {
/// The files a CSV merge read, each of which gets its own view. Empty for /// The files a CSV merge read, each of which gets its own view. Empty for
/// a single file or a JSON import (improvise-lpzu). /// a single file or a JSON import (improvise-lpzu).
pub sources: Vec<SourceFile>, pub sources: Vec<SourceFile>,
/// What the records' string values are, fixed by the constructor (8jk6.4).
scalars: Scalars,
}
/// How a record's values read. Why CSV is text: `context/code/formula-and-io.md`.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Scalars {
/// JSON: a string is text and a number is a number.
Typed,
/// CSV: every value is source text, read as a number only where one is wanted.
Text,
}
impl Scalars {
/// The record as analysis sees it: typed, whatever its source.
fn typed_record(self, record: &Value) -> Value {
match (self, record) {
(Scalars::Text, Value::Object(map)) => Value::Object(
map.iter()
.map(|(k, v)| (k.clone(), csv_parser::typed(v)))
.collect(),
),
(Scalars::Text, other) | (Scalars::Typed, other) => other.clone(),
}
}
/// The value a measure takes from `value`, if it is a number.
fn number(self, value: &Value) -> Option<f64> {
match self {
Scalars::Typed => value.as_f64(),
Scalars::Text => value
.as_str()
.and_then(csv_parser::number_in)
.and_then(|n| n.as_f64()),
}
}
} }
/// Several source records that land on one coordinate tuple (improvise-cgqt). /// Several source records that land on one coordinate tuple (improvise-cgqt).
@@ -57,7 +93,17 @@ pub enum DuplicatePolicy {
} }
impl ImportPipeline { impl ImportPipeline {
/// A pipeline over a JSON value.
pub fn new(raw: Value) -> Self { pub fn new(raw: Value) -> Self {
Self::reading(raw, Scalars::Typed)
}
/// A pipeline over CSV records, whose values are their source text.
pub fn from_csv(records: CsvRecords) -> Self {
Self::reading(Value::Array(records.into_values()), Scalars::Text)
}
fn reading(raw: Value, scalars: Scalars) -> Self {
let array_paths = find_array_paths(&raw); let array_paths = find_array_paths(&raw);
let mut pipeline = Self { let mut pipeline = Self {
raw: raw.clone(), raw: raw.clone(),
@@ -69,6 +115,7 @@ impl ImportPipeline {
formulas: vec![], formulas: vec![],
on_duplicate: DuplicatePolicy::Refuse, on_duplicate: DuplicatePolicy::Refuse,
sources: vec![], sources: vec![],
scalars,
}; };
// Auto-select if root is an array or there is exactly one candidate path. // Auto-select if root is an array or there is exactly one candidate path.
@@ -86,7 +133,46 @@ impl ImportPipeline {
self.selected_path = path.to_string(); self.selected_path = path.to_string();
if let Some(arr) = extract_array_at_path(&self.raw, path) { if let Some(arr) = extract_array_at_path(&self.raw, path) {
self.records = arr.clone(); self.records = arr.clone();
self.proposals = analyze_records(&self.records); self.analyze();
}
}
/// Propose a kind for every field, reading the records as `scalars` says.
pub fn analyze(&mut self) {
let typed: Vec<Value> = self
.records
.iter()
.map(|r| self.scalars.typed_record(r))
.collect();
self.proposals = analyze_records(&typed);
}
/// Why there is nothing to build, when no record is an object to place.
pub fn nothing_to_import(&self) -> Option<String> {
let total = self.records.len();
match (total, self.records.iter().any(Value::is_object)) {
(_, true) => None,
(0, false) => Some("Nothing to import: the source has no records".into()),
(_, false) => Some(format!(
"Nothing to import: all {total} records were skipped as not JSON objects"
)),
}
}
/// The formulas, parsed. Every one that does not parse is named with its
/// error, never dropped (improvise-8jk6.5).
pub fn parsed_formulas(&self) -> Result<Vec<Formula>> {
let mut parsed = vec![];
let mut refused: Vec<String> = vec![];
for raw in &self.formulas {
match import_formula(raw) {
Ok(formula) => parsed.push(formula),
Err(e) => refused.push(e.to_string()),
}
}
match refused.is_empty() {
true => Ok(parsed),
false => Err(anyhow!(refused.join("\n"))),
} }
} }
@@ -201,6 +287,7 @@ impl ImportPipeline {
measures: accepted(&[FieldKind::Measure]), measures: accepted(&[FieldKind::Measure]),
labels: accepted(&[FieldKind::Label]), labels: accepted(&[FieldKind::Label]),
date_extractions, date_extractions,
scalars: self.scalars,
} }
} }
@@ -211,9 +298,13 @@ impl ImportPipeline {
let categories = &plan.categories; let categories = &plan.categories;
let date_extractions = &plan.date_extractions; let date_extractions = &plan.date_extractions;
if let Some(why) = self.nothing_to_import() {
return Err(anyhow!(why));
}
if categories.is_empty() { if categories.is_empty() {
return Err(anyhow!("At least one category must be accepted")); return Err(anyhow!("At least one category must be accepted"));
} }
let formulas = self.parsed_formulas()?;
let placements: Vec<Placement> = self.records.iter().map(|r| plan.place(r)).collect(); let placements: Vec<Placement> = self.records.iter().map(|r| plan.place(r)).collect();
let collisions = collisions_among(&placements); let collisions = collisions_among(&placements);
@@ -326,13 +417,8 @@ impl ImportPipeline {
wb.model.set_cell(cell.key, CellValue::Number(cell.total)); wb.model.set_cell(cell.key, CellValue::Number(cell.total));
} }
// Parse and add formulas for formula in formulas {
// Formulas target the "_Measure" category by default. wb.model.add_formula(formula);
let formula_cat: String = "_Measure".to_string();
for raw in &self.formulas {
if let Ok(formula) = parse_formula(raw, &formula_cat) {
wb.model.add_formula(formula);
}
} }
for view in source_views(&wb, &self.sources, &plan) { for view in source_views(&wb, &self.sources, &plan) {
@@ -410,6 +496,7 @@ struct Plan<'a> {
measures: Vec<&'a FieldProposal>, measures: Vec<&'a FieldProposal>,
labels: Vec<&'a FieldProposal>, labels: Vec<&'a FieldProposal>,
date_extractions: Vec<(&'a str, &'a str, DateComponent, String)>, date_extractions: Vec<(&'a str, &'a str, DateComponent, String)>,
scalars: Scalars,
} }
/// Where one source record lands. Worked out once, for the build and for the /// Where one source record lands. Worked out once, for the build and for the
@@ -466,7 +553,7 @@ impl Plan<'_> {
let measures = self let measures = self
.measures .measures
.iter() .iter()
.filter_map(|m| Some((m.field.clone(), map.get(&m.field)?.as_f64()?))) .filter_map(|m| Some((m.field.clone(), self.scalars.number(map.get(&m.field)?)?)))
.collect(); .collect();
Placement::Placed { Placement::Placed {
coords, coords,
@@ -582,6 +669,11 @@ fn unparsed_date_warnings(
.collect() .collect()
} }
/// An import formula, targeting `_Measure`, or the error naming it (improvise-8jk6.5).
fn import_formula(raw: &str) -> Result<Formula> {
parse_formula(raw, "_Measure").map_err(|e| anyhow!("Formula error in '{raw}': {e}"))
}
/// The warning for records that could not be placed in the grid. A record /// 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. /// missing a category value is placed, so only a non-object is skipped.
fn skip_warnings(total: usize, skipped_non_objects: usize) -> Option<String> { fn skip_warnings(total: usize, skipped_non_objects: usize) -> Option<String> {
@@ -769,7 +861,9 @@ impl ImportWizard {
fn validation_error(&self) -> Option<String> { fn validation_error(&self) -> Option<String> {
match self.step { match self.step {
WizardStep::ReviewProposals => self WizardStep::ReviewProposals => self
.no_category_message() .pipeline
.nothing_to_import()
.or_else(|| self.no_category_message())
.or_else(|| self.category_limit_message()), .or_else(|| self.category_limit_message()),
WizardStep::ConfigureDates => self.category_limit_message(), WizardStep::ConfigureDates => self.category_limit_message(),
WizardStep::ResolveDuplicates => match &self.pipeline.on_duplicate { WizardStep::ResolveDuplicates => match &self.pipeline.on_duplicate {
@@ -1064,14 +1158,24 @@ impl ImportWizard {
self.formula_buffer.pop(); self.formula_buffer.pop();
} }
/// Commit the current formula buffer to the pipeline's formula list. /// Commit the current formula buffer to the pipeline's formula list. One
/// that does not parse stays in the buffer, with the error (improvise-8jk6.5).
pub fn confirm_formula(&mut self) { pub fn confirm_formula(&mut self) {
let text = self.formula_buffer.trim().to_string(); let text = self.formula_buffer.trim().to_string();
if !text.is_empty() { match (text.is_empty(), import_formula(&text)) {
self.pipeline.formulas.push(text); (false, Err(e)) => self.message = Some(e.to_string()),
(false, Ok(_)) => {
self.pipeline.formulas.push(text);
self.close_formula_edit();
}
(true, _) => self.close_formula_edit(),
} }
}
fn close_formula_edit(&mut self) {
self.formula_buffer.clear(); self.formula_buffer.clear();
self.formula_editing = false; self.formula_editing = false;
self.message = None;
self.cursor = self.pipeline.formulas.len().saturating_sub(1); self.cursor = self.pipeline.formulas.len().saturating_sub(1);
} }
@@ -2176,7 +2280,7 @@ mod tests {
let paths: Vec<&str> = files.iter().map(|(p, _)| *p).collect(); let paths: Vec<&str> = files.iter().map(|(p, _)| *p).collect();
let merged = merge_csvs_from(&fs, &paths).unwrap(); let merged = merge_csvs_from(&fs, &paths).unwrap();
assert!(merged.warnings.is_empty(), "{:?}", merged.warnings); assert!(merged.warnings.is_empty(), "{:?}", merged.warnings);
let mut p = ImportPipeline::new(Value::Array(merged.records)); let mut p = ImportPipeline::from_csv(merged.records);
p.sources = merged.sources; p.sources = merged.sources;
p p
} }
@@ -2193,8 +2297,11 @@ mod tests {
), ),
]; ];
fn code_maat_records() -> Value { /// The code-maat merge without its sources, so no per-file views.
Value::Array(merged_pipeline(&CODE_MAAT).records) fn code_maat_without_views() -> ImportPipeline {
let mut p = merged_pipeline(&CODE_MAAT);
p.sources.clear();
p
} }
/// Every non-empty cell a view's grid draws, row by row. /// Every non-empty cell a view's grid draws, row by row.
@@ -2360,7 +2467,7 @@ mod tests {
/// 0, "" or a skip. Evidence: `context/test-evidence/formula-and-io.md`. /// 0, "" or a skip. Evidence: `context/test-evidence/formula-and-io.md`.
#[test] #[test]
fn files_with_different_headers_import_as_one_sparse_model() { fn files_with_different_headers_import_as_one_sparse_model() {
let p = ImportPipeline::new(code_maat_records()); let p = code_maat_without_views();
let kinds: Vec<(&str, FieldKind)> = p let kinds: Vec<(&str, FieldKind)> = p
.proposals .proposals
.iter() .iter()
@@ -2402,7 +2509,7 @@ mod tests {
/// "". Evidence: `context/test-evidence/formula-and-io.md`. /// "". Evidence: `context/test-evidence/formula-and-io.md`.
#[test] #[test]
fn a_label_column_a_record_lacks_is_no_coordinate_not_an_empty_item() { fn a_label_column_a_record_lacks_is_no_coordinate_not_an_empty_item() {
let mut p = ImportPipeline::new(code_maat_records()); let mut p = code_maat_without_views();
p.proposals p.proposals
.iter_mut() .iter_mut()
.find(|f| f.field == "coupled") .find(|f| f.field == "coupled")
@@ -3005,4 +3112,98 @@ mod tests {
w.message w.message
); );
} }
/// improvise-8jk6.5. Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn a_formula_that_does_not_parse_stops_the_build_and_is_named() {
let mut p = ImportPipeline::new(json!([{"region": "East", "revenue": 1.0}]));
p.formulas = vec![
"Good = revenue".into(),
"Bad = revenue +".into(),
"Worse = * 2".into(),
];
let err = p.build_model().expect_err("an unparseable formula refuses");
let msg = err.to_string();
assert!(msg.contains("Formula error in 'Bad = revenue +'"), "{msg}");
assert!(msg.contains("Formula error in 'Worse = * 2'"), "{msg}");
assert!(!msg.contains("Good"), "{msg}");
}
/// improvise-8jk6.5: the wizard refuses where the text can still be
/// fixed. Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn confirming_a_formula_that_does_not_parse_keeps_it_out_and_the_text_in() {
let mut w = sample_wizard();
w.start_formula_edit();
for c in "Bad = revenue +".chars() {
w.push_formula_char(c);
}
w.confirm_formula();
assert!(w.pipeline.formulas.is_empty(), "{:?}", w.pipeline.formulas);
assert_eq!(w.formula_buffer, "Bad = revenue +");
assert!(w.formula_editing);
let message = w.message.clone().unwrap_or_default();
assert!(
message.starts_with("Formula error in 'Bad = revenue +'"),
"{message}"
);
}
/// improvise-8jk6.4: the role reads the text. Evidence:
/// `context/test-evidence/formula-and-io.md`.
#[test]
fn a_csv_column_read_as_a_category_keeps_its_text_and_as_a_measure_its_number() {
use crate::import::csv_parser::parse_csv_bytes_with_warnings;
use crate::model::cell::{CellKey, CellValue};
let csv = b"commit,lines\n00565796,3\n220944e2,5\n";
let (records, _) = parse_csv_bytes_with_warnings(csv).unwrap();
let mut p = ImportPipeline::from_csv(records);
let kind = |p: &ImportPipeline, f: &str| {
p.proposals
.iter()
.find(|x| x.field == f)
.unwrap()
.kind
.clone()
};
assert_eq!(kind(&p, "commit"), FieldKind::Measure, "analysis types");
let commit = p.proposals.iter_mut().find(|x| x.field == "commit");
commit.unwrap().kind = FieldKind::Category;
let wb = p.build_model().unwrap();
let items: Vec<&String> = wb.model.category("commit").unwrap().items.keys().collect();
assert_eq!(items, ["00565796", "220944e2"]);
let key = CellKey::new(vec![
("_Measure".into(), "lines".into()),
("commit".into(), "220944e2".into()),
]);
assert_eq!(wb.model.get_cell(&key), Some(&CellValue::Number(5.0)));
}
/// improvise-8jk6.6: the wizard asks on the review step, where another
/// path can still be chosen. Evidence: the test-evidence doc.
#[test]
fn the_review_step_says_when_no_record_can_be_imported() {
let mut w = ImportWizard::new(json!([1, 2]));
assert_eq!(w.step, WizardStep::ReviewProposals, "setup");
w.advance();
assert_eq!(w.step, WizardStep::ReviewProposals);
assert_eq!(
w.message.as_deref(),
Some("Nothing to import: all 2 records were skipped as not JSON objects")
);
}
/// improvise-8jk6.6. Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn a_build_that_keeps_no_record_refuses_and_says_why() {
let skipped = ImportPipeline::new(json!([1, "two", null]));
let msg = skipped.build_model().expect_err("refuses").to_string();
assert_eq!(
msg,
"Nothing to import: all 3 records were skipped as not JSON objects"
);
let empty = ImportPipeline::new(json!([]));
let msg = empty.build_model().expect_err("refuses").to_string();
assert_eq!(msg, "Nothing to import: the source has no records");
}
} }
@@ -842,7 +842,7 @@ fn a_csv_header_with_an_embedded_newline_survives_a_save() {
let csv = "\"Region\nName\",Amount\nEast,1\nWest,2\n"; let csv = "\"Region\nName\",Amount\nEast,1\nWest,2\n";
let (records, _) = let (records, _) =
crate::import::csv_parser::parse_csv_bytes_with_warnings(csv.as_bytes()).unwrap(); crate::import::csv_parser::parse_csv_bytes_with_warnings(csv.as_bytes()).unwrap();
let wb = crate::import::wizard::ImportPipeline::new(serde_json::Value::Array(records)) let wb = crate::import::wizard::ImportPipeline::from_csv(records)
.build_model() .build_model()
.unwrap(); .unwrap();
assert!( assert!(
+2 -2
View File
@@ -360,7 +360,7 @@ fn get_import_data(args: &[PathBuf]) -> Option<ImportPipeline> {
for warning in &merged.warnings { for warning in &merged.warnings {
eprintln!("warning: {warning}"); eprintln!("warning: {warning}");
} }
let mut pipeline = ImportPipeline::new(Value::Array(merged.records)); let mut pipeline = ImportPipeline::from_csv(merged.records);
pipeline.sources = merged.sources; pipeline.sources = merged.sources;
Some(pipeline) Some(pipeline)
} }
@@ -379,7 +379,7 @@ fn get_import_data(args: &[PathBuf]) -> Option<ImportPipeline> {
Ok(content) => { Ok(content) => {
if csv_path_p(path) { if csv_path_p(path) {
match crate::import::csv_parser::parse_csv(path) { match crate::import::csv_parser::parse_csv(path) {
Ok(records) => Some(ImportPipeline::new(Value::Array(records))), Ok(records) => Some(ImportPipeline::from_csv(records)),
Err(e) => { Err(e) => {
eprintln!("CSV parse error: {e}"); eprintln!("CSV parse error: {e}");
None None
+5 -5
View File
@@ -63,8 +63,8 @@ fn duplicate_csv_headers_not_silent() {
2, 2,
"both columns must survive as separate fields: {fields:?}" "both columns must survive as separate fields: {fields:?}"
); );
assert_eq!(fields["A"].as_f64(), Some(1.0), "{fields:?}"); assert_eq!(fields["A"], "1", "{fields:?}");
assert_eq!(fields["A_2"].as_f64(), Some(2.0), "{fields:?}"); assert_eq!(fields["A_2"], "2", "{fields:?}");
assert!( assert!(
warnings warnings
.iter() .iter()
@@ -77,7 +77,7 @@ fn duplicate_csv_headers_not_silent() {
fn blank_measure_still_infers_numeric() { fn blank_measure_still_infers_numeric() {
use improvise_io::import::{analyzer::FieldKind, wizard::ImportPipeline}; use improvise_io::import::{analyzer::FieldKind, wizard::ImportPipeline};
let (records, _) = parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nWest,\n").unwrap(); let (records, _) = parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nWest,\n").unwrap();
let p = ImportPipeline::new(serde_json::Value::Array(records)); let p = ImportPipeline::from_csv(records);
let proposal = p.proposals.iter().find(|p| p.field == "Revenue").unwrap(); let proposal = p.proposals.iter().find(|p| p.field == "Revenue").unwrap();
assert_eq!(proposal.kind, FieldKind::Measure); assert_eq!(proposal.kind, FieldKind::Measure);
} }
@@ -86,7 +86,7 @@ fn repeated_import_coordinates_not_silently_overwritten() {
use improvise_io::import::wizard::ImportPipeline; use improvise_io::import::wizard::ImportPipeline;
let (records, _) = let (records, _) =
parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nEast,20\n").unwrap(); parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nEast,20\n").unwrap();
let mut pipeline = ImportPipeline::new(serde_json::Value::Array(records)); let mut pipeline = ImportPipeline::from_csv(records);
pipeline.on_duplicate = improvise_io::import::wizard::DuplicatePolicy::Sum; pipeline.on_duplicate = improvise_io::import::wizard::DuplicatePolicy::Sum;
let (wb, warnings) = pipeline.build_model_with_warnings().unwrap(); let (wb, warnings) = pipeline.build_model_with_warnings().unwrap();
let values: Vec<f64> = wb let values: Vec<f64> = wb
@@ -399,7 +399,7 @@ fn ordinary_controls() {
let (records, warnings) = let (records, warnings) =
parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nWest,20\n").unwrap(); parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nWest,20\n").unwrap();
assert!(warnings.is_empty()); assert!(warnings.is_empty());
let p = ImportPipeline::new(serde_json::Value::Array(records)); let p = ImportPipeline::from_csv(records);
assert_eq!( assert_eq!(
p.proposals p.proposals
.iter() .iter()
+1 -1
View File
@@ -25,7 +25,7 @@ fn reloaded_folder_import() -> App {
.unwrap(); .unwrap();
} }
let merged = merge_csvs_from(&fs, &files.map(|(p, _)| p)).unwrap(); let merged = merge_csvs_from(&fs, &files.map(|(p, _)| p)).unwrap();
let mut pipeline = ImportPipeline::new(serde_json::Value::Array(merged.records)); let mut pipeline = ImportPipeline::from_csv(merged.records);
pipeline.sources = merged.sources; pipeline.sources = merged.sources;
let (wb, _) = pipeline.build_model_with_warnings().unwrap(); let (wb, _) = pipeline.build_model_with_warnings().unwrap();
App::new(parse_md(&format_md(&wb)).unwrap(), None) App::new(parse_md(&format_md(&wb)).unwrap(), None)
+114
View File
@@ -708,3 +708,117 @@ fn a_headless_import_takes_files_and_folders_together_and_refuses_an_empty_folde
); );
assert!(!scratch.path("never.improv").exists()); assert!(!scratch.path("never.improv").exists());
} }
/// Runs `import <source> --no-wizard <extra> -o out.improv` on a source it writes.
fn import_of(scratch: &Scratch, name: &str, body: &str, extra: &[&str]) -> (Output, PathBuf) {
let source = scratch.path(name);
std::fs::write(&source, body).unwrap();
let out_path = scratch.path("out.improv");
(run_import(&source, &out_path, extra), out_path)
}
/// improvise-8jk6.4: a forced category's values are item names, so they keep
/// their source text. Evidence: `context/test-evidence/cli.md`.
#[test]
fn a_csv_column_forced_to_category_keeps_its_text_verbatim() {
let scratch = Scratch::new("forced_category_text");
let (out, out_path) = import_of(
&scratch,
"commits.csv",
"commit,lines\n00565796,3\n220944e2,5\n",
&["--category", "commit", "--measure", "lines"],
);
assert!(out.status.success(), "{}", stderr_of(&out));
let doc = std::fs::read_to_string(&out_path).unwrap();
assert!(
doc.lines().any(|l| l == "- |00565796|, |220944e2|"),
"the items are the source text:\n{doc}"
);
for line in [
"_Measure=lines, commit=|00565796| = 3",
"_Measure=lines, commit=|220944e2| = 5",
] {
assert!(doc.lines().any(|l| l == line), "{line} missing from\n{doc}");
}
}
/// improvise-8jk6.4: JSON has no source text for a number, so a forced
/// category names it as serde_json writes it. Evidence file.
#[test]
fn a_json_number_forced_to_category_is_named_as_json_writes_it() {
let scratch = Scratch::new("forced_category_json");
let (out, out_path) = import_of(
&scratch,
"commits.json",
r#"[{"commit": 42, "n": 1}, {"commit": 1.5, "n": 2}, {"commit": 1e3, "n": 3}]"#,
&["--category", "commit", "--measure", "n"],
);
assert!(out.status.success(), "{}", stderr_of(&out));
let doc = std::fs::read_to_string(&out_path).unwrap();
assert!(
doc.lines().any(|l| l == "- |42|, |1.5|, |1000.0|"),
"the items are the numbers' JSON text:\n{doc}"
);
}
/// improvise-8jk6.5: a formula the user named and that does not parse is a
/// refusal, not a silent drop. Evidence: `context/test-evidence/cli.md`.
#[test]
fn an_import_formula_that_does_not_parse_refuses_names_it_and_writes_nothing() {
let scratch = Scratch::new("bad_formula");
let (out, out_path) = import_of(
&scratch,
"f.csv",
"a,x\nk,1\n",
&[
"--measure",
"x",
"--formula",
"Y = x +",
"--formula",
"Z = x",
],
);
let err = stderr_of(&out);
assert!(!out.status.success(), "a dropped formula exited 0: {err}");
assert!(err.contains("'Y = x +'"), "names the formula: {err}");
assert!(err.contains("expected"), "carries the parse error: {err}");
assert!(!err.contains("'Z = x'"), "names only the bad one: {err}");
assert!(!out_path.exists(), "nothing is written");
}
/// improvise-8jk6.6: an import that kept no record says why, and exits
/// non-zero writing nothing. Evidence: `context/test-evidence/cli.md`.
#[test]
fn an_import_that_keeps_no_record_refuses_and_says_why() {
let scratch = Scratch::new("kept_none");
let (out, out_path) = import_of(&scratch, "r.json", r#"[1, 2, "three"]"#, &[]);
let err = stderr_of(&out);
assert!(!out.status.success(), "{err}");
assert!(
err.contains("all 3 records were skipped as not JSON objects"),
"says why: {err}"
);
assert!(!out_path.exists(), "nothing is written");
}
/// improvise-8jk6.6: a partial skip warns with a count and exits 0, a choice
/// put to the user. Evidence: `context/test-evidence/cli.md`.
#[test]
fn an_import_that_skips_some_records_warns_with_a_count_and_still_writes() {
let scratch = Scratch::new("kept_some");
let (out, out_path) = import_of(
&scratch,
"p.json",
r#"[{"a": "k", "x": 1}, 2, {"a": "m", "x": 4}]"#,
&[],
);
let err = stderr_of(&out);
assert!(out.status.success(), "{err}");
assert!(
err.contains("warning: Skipped 1 of 3 records that were not JSON objects"),
"{err}"
);
let doc = std::fs::read_to_string(&out_path).unwrap();
assert!(doc.contains("_Measure=x, a=m = 4"), "{doc}");
}
+1 -1
View File
@@ -18,7 +18,7 @@ fn imported_model_shows_one_row_per_record_with_measure_columns() {
.expect("write csv"); .expect("write csv");
let records = improvise::import::csv_parser::parse_csv(&csv).expect("csv parses"); let records = improvise::import::csv_parser::parse_csv(&csv).expect("csv parses");
let pipeline = ImportPipeline::new(serde_json::Value::Array(records)); let pipeline = ImportPipeline::from_csv(records);
let workbook = pipeline.build_model().expect("model builds"); let workbook = pipeline.build_model().expect("model builds");
let saved = dir.path().join("w.improv"); let saved = dir.path().join("w.improv");