diff --git a/README.md b/README.md index dc00f091..b4a4b357 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ improvise import examples/demo.csv \ --measure Revenue --measure Cost \ --time Date --extract Date:Month --split-date Date \ --axis Region:row --axis Product:row \ - --axis Date_Month:column --axis Measure:column \ + --axis Date_Month:column --axis _Measure:column \ --axis Customer:page --axis Date:none \ --formula "Profit = Revenue - Cost" \ --name "Acme Sales Demo" \ diff --git a/context/code/cli.md b/context/code/cli.md index 8fecbd1d..5add820a 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -392,3 +392,36 @@ 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. + +## 13 — a headless import refuses rather than saving less than was asked (improvise-8jk6.4, 8jk6.5, 8jk6.6) + +The user treats the headless command and model layer as a product of its own, +and the scripted `import --no-wizard` is where it meets a pipeline. The +repo-stats pipeline (improvise-1xfb.5.1) found three ways that import saved +something other than what the flags asked for, and still exited 0. The +reasoning for each is in `context/code/formula-and-io.md`, "A CSV field is +text, and the field's role reads it" and "An import refuses what it cannot +keep". The CLI's contract: + +- **`--category` keeps the CSV text.** A forced category's items are the + file's text, verbatim: `00565796` stays `00565796`, and `220944e2` stays + `220944e2`. Measures still read as numbers. A JSON number has no source + text, so it is named as JSON writes it (`1e3` is `1000.0`). +- **An unparseable `--formula` is a refusal.** It exits non-zero and writes + nothing. Every bad formula is named on stderr, each with its parse error. + This is §10's rule: the request was not met, so the exit code says so. The + wizard refuses the same text at the formula step, with the same wording. +- **An import that kept no record is a refusal.** It exits non-zero, writes + nothing, and says why: every record was skipped as not a JSON object, or + the source had none. +- **An import that skipped *some* records exits 0 by default.** It writes the + model and warns with a count. With `--strict` (the user's ruling on PR #321) + any skipped record is a refusal instead: exit 1, the same count named, and + nothing written. The argument is in `formula-and-io.md`. + +Two refusals bail in `run_headless_import` before the build: the `--strict` +skip check (`src/main.rs:305`) and the `--on-duplicate refuse` collision +check. The rest come from `build_model_with_warnings` as an `Err`. All of +them reach `main` as exit 1 before `persistence::save`. So "writes nothing" +holds by statement order, not by where the refusal is raised. The headless +tests check it by asserting that the output path does not exist. diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 1d3bd443..9a8b88ad 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -711,6 +711,118 @@ one proves nothing. backend rather than only a file (improvise-fuj); the path-taking wrappers remain for the native CLI. +### A CSV field is text, and the field's role reads it (improvise-8jk6.4) + +The parser used to type every field as it read it: text that parsed as an +`i64` or a finite `f64` became a `Value::Number`. That is a lossy step, and it +ran before anyone knew what the column was for. A column the user then forced +to Category (`--category`, or the wizard's `c`) took its item names from the +number's display, not from the file. So commit `00565796` became item `565796`, +and `220944e2` was read as scientific notation and became `22094400.0`. The +repo-stats pipeline (improvise-1xfb.5.1) found it. + +CSV has no types. Whether `00565796` is a number depends on whether its column +is a measure, and the parser cannot know that. So the parser now yields every +non-empty field as a `Value::String` of its text (blank is still `Null`), and +two readers apply the old numeric rule, `csv_parser::number_in`, only where a +number is wanted: + +- **Analysis** proposes a kind from a typed copy of the records + (`Scalars::typed_record`), so a numeric CSV column is still proposed as a + Measure, exactly as before. +- **A measure** takes its value through `Scalars::number`. + +A category or a label is named from the text itself, so it is verbatim. + +`ImportPipeline` carries the reading as a private `scalars: Scalars`: +`Typed` for JSON and `Text` for CSV. JSON cannot share the text rule, because +a JSON string is text by the file's own say. `"02139"` in JSON is a zip code, +and reading it as a number would propose it as a Measure. A JSON number +forced to Category has no source text to keep, because serde_json does not +keep one. It is named by `Value`'s own display, which is what writing it back +as JSON would say: `42`, `1.5`, and `1000.0` for `1e3`. + +**The parser returns `CsvRecords`, not `Vec`.** It derefs to +`[Value]`, so a caller can read it. It converts to a `Vec` only inside the +crate, and the only public way into a pipeline is `ImportPipeline::from_csv`. +This is not caution for its own sake. When the parser first changed, eleven +tests in four files failed. Every one handed parser output to +`ImportPipeline::new`, the JSON constructor, which would read every column as +text and propose `Revenue` as a Category. Production had the same shape at all +three CSV entry points: `main.rs`'s single file, `ImportPipeline::from_merge` +(the merge path, from PR #314), and `ImportJsonHeadless`. `from_merge` merged +in with `Self::new` and now calls `Self::from_csv`. Now that mistake does not +compile. Two holes remain, and +both take a deliberate step. `ImportPipeline::records` is still a public +`Vec`, and a test helper once copied it out of a CSV pipeline into a +JSON one. And `CsvRecords` derefs to a slice that `.to_vec()` will copy. + +### An import refuses what it cannot keep (improvise-8jk6.5, improvise-8jk6.6) + +Two failures used to end in an import that saved and reported success. + +**A formula that does not parse.** `build_model_with_warnings` ran +`if let Ok(formula) = parse_formula(..)`, so a bad `--formula` vanished. The +headless run then saved and exited 0, and the wizard kept the bad text in its +list and dropped it the same way at build. Both now go through one +function, `import_formula`, whose error names the formula and carries pest's +parse error: `Formula error in 'Y = x +': …`. The prefix matches the TUI +formula minibuffer's `Formula error:`. + +- `ImportPipeline::parsed_formulas` parses them all. If any fails, it refuses + and names **every** formula that failed, one per line (a message the user + needs is never truncated). The build calls it before creating anything, so a + refusal writes nothing. +- The wizard checks at `confirm_formula`, the one place the user can still fix + the text (gotcha 22). A formula that does not parse stays in the buffer with + the error as the step's message, and never enters the list. + +**Nothing kept.** The one skip left after improvise-8jk6.2 is a record that +is not a JSON object. When every record is skipped, there are no fields, so +there are no proposals and no categories. The build did refuse that case, but +by accident, with "At least one category must be accepted". That tells the +user to change a field's kind when there are no fields. The ticket's own repro +(`[1, 2, "three"]`) already exited 1 on the lpzu base, so the headline did not +reproduce there. What did reproduce was the reason. `nothing_to_import` now +states the rule and runs before the category check: `Nothing to import: all 3 +records were skipped as not JSON objects`, or `the source has no records`. The +wizard asks it first on `ReviewProposals`, where another array path can still +be chosen. + +One case meets PR #314's "a header-only file is still a file". A folder whose +CSVs are **all** header-only has no records, only sources. Built from +`from_merge`'s seeded proposals alone, it would save `File` items, empty +columns and one empty records view per file. It is refused here, because it +kept no record. A header-only file *beside* files with rows is unaffected, +since some record was kept. The user confirmed this on PR #321: "Refuse (as +#321 does)". + +**A partial skip stays a warning.** Some records kept and some skipped still +builds, exits 0, and warns with a count: `Skipped 1 of 3 records that were not +JSON objects`. The argument: a non-object in an array of records is not a +record that failed to import. It is structurally not a record, such as a +trailing `null` or a stray scalar in a hand-edited file. Every record that +*is* a record was kept, and the count is on stderr for a script to read. +Making it fatal would fail every import that carries one stray element. +Making it a distinct non-zero exit that still writes gives a contract where +`import … && next` stops even though the file was written. + +The user ruled on PR #321: "Add a --strict flag". The default stays exit 0 +with the counted warning, pinned by +`an_import_that_skips_some_records_warns_with_a_count_and_still_writes`. +`import --no-wizard --strict` turns any skipped record into a refusal: +`--strict: Skipped 1 of 3 records that were not JSON objects`, exit 1. + +**A strict refusal writes nothing.** A refusal that still wrote the file would +be the half-met contract argued against above: `&&` stops while the output +sits there, and a script that checks only the file gets a model that `--strict` +existed to prevent. So the check runs in `run_headless_import` before the +build. It reads `ImportPipeline::skip_warning`, the same function the build's +warning comes from, so the two cannot disagree about what was skipped. +"Skipped" here means a record that is not an object, the only skip left after +8jk6.2. An unparsed date or a short CSV row is a warning about a record that +was kept, and `--strict` does not refuse it. + ### 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 diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 96ee4f9e..7f9b2233 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -448,6 +448,57 @@ 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_csv_column_forced_to_category_keeps_its_text_verbatim + +improvise-8jk6.4. Before the fix, `commit,lines` with `00565796` and +`220944e2` forced to Category imported as items `565796` and `22094400.0`. +The CSV parser had typed both as numbers, and the category was named from the +number. On the base this failed with the items line `- |565796|, |22094400.0|`. +The test also asserts both Data lines, which proves the measure `lines` still +reads 3 and 5 through the same records. + +- **R1**, a category named from the typed value, fails it. +- **R2**, a CSV measure read as a JSON value (`as_f64` on the text), fails it + on the Data lines. + +### a_json_number_forced_to_category_is_named_as_json_writes_it + +improvise-8jk6.4, the JSON half. This is not a bug demonstration: it passed +on the base, and it pins that the fix left JSON alone. JSON keeps no source +text for a number, so `42`, `1.5` and `1e3` name items `42`, `1.5` and +`1000.0`. `1e3` is the discriminating value, because its source text, its f64 +display (`1000`) and its JSON display (`1000.0`) all differ. **R10**, naming a +number by its `f64` display, fails it on `1000.0`. + +### an_import_formula_that_does_not_parse_refuses_names_it_and_writes_nothing + +improvise-8jk6.5. Before the fix, `--formula 'Y = x +'` was dropped: the run +printed `Saved to …` and exited 0. That is the base failure (`a dropped +formula exited 0`). The test passes a good formula too (`Z = x`) and asserts +it is not named, so a refusal that lists every formula cannot pass. +`expected` comes from pest's error, which proves the parse error is carried +and not only the formula text. **R4**, a build that drops what does not +parse, fails it. Since improvise-8jk6.17 it passes two bad formulas, so +**R5**, naming only the first, fails it here too. At `efaa5050` only the io +unit test saw R5. + +### an_import_that_keeps_no_record_refuses_and_says_why + +improvise-8jk6.6. On the lpzu base `[1, 2, "three"]` already exited 1, so the +exit-code half was not the bug. The reason was: `At least one category must +be accepted`, which is true only because no fields exist, and which points +the user at a fix that cannot help. The base failure was on the message. +**R7**, removing `nothing_to_import` from the build, fails it the same way. + +### an_import_that_skips_some_records_warns_with_a_count_and_still_writes + +improvise-8jk6.6, the partial case. It passed on the base. It pins the +default the user kept on PR #321 ("Add a --strict flag"), so that changing +the default is a decision and not a drift. It asserts the warning's count (`1 of 3`), exit 0, +and a Data line from the record *after* the skipped one. That last check +shows the skip did not end the import early. **R9**, making any skip fatal, +fails 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"). @@ -458,3 +509,42 @@ 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". + +### a_strict_import_that_skips_a_record_refuses_and_writes_nothing + +improvise-8jk6.6, the user's ruling on PR #321. Written before `--strict` +existed, it failed on clap's unknown-argument error. It asserts the refusal +names the flag and the same count as the default warning, and that the output +path does not exist. + +- **S1**, the flag parsed but ignored, fails it on the exit status. +- **S3**, refusing only after `persistence::save`, fails it on the output + path. That is the "a strict refusal still writes" contract this PR decided + against. + +### a_strict_import_that_skips_nothing_still_writes + +The control for the entry above. Without it, a `--strict` that refuses +**every** import passes that test. **S2** is that rival, and it fails here. +It failed before the flag existed too, but only on clap's error, which is not +the discriminating failure. The discriminating one is S2. + +### a_csv_decimal_measure_imports_as_its_number + +improvise-8jk6.17 (F3), from the adversarial review of #321. The fixture has +decimal CSV measures (`1000.50`, `-2.25`). With `number_in`'s `f64` branch +disabled (integers only), no headless test failed at `efaa5050`, and a decimal +measure was silently dropped. **F3** now fails this test and the next one. +This is coverage, not a bug: it passed when written. + +### import_json_on_a_csv_reads_its_numeric_column_as_a_measure + +improvise-8jk6.17 (F1). `improvise cmd 'import-json f.csv'` reaches +`ImportJsonHeadless`, a third CSV entry point besides `import`. **F1** +restores its CSV branch to the JSON constructor via +`ImportPipeline::new(Value::Array(recs.to_vec()))`. That still compiles, +because `CsvRecords` derefs to a slice. `Revenue` then becomes a category of +text items, and this test fails on its Data lines. At `efaa5050` only +`import_headless_refuses_records_that_collide` failed under F1, and only by +accident. The `## Category: Revenue` assertion names the symptom directly. +This is coverage: it passed when written. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 5523b05a..5284af34 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -882,6 +882,11 @@ The control: a **lone** `\r` is itself a record terminator to this reader, so it is not part of a CRLF pair, and it is why "two records, clean values" is a real observation rather than a coincidence. +Since improvise-8jk6.4 the parser yields text, so the assertion is that +`Revenue` is the text `1`. A kept `\r` would make it `1\r`, which fails that +assertion, and which `number_in` would not read as a number where a measure +wanted one. + ### an_empty_file_is_no_records_and_no_warnings An empty file is no records and no complaint. The wizard's own preconditions @@ -919,12 +924,17 @@ The consequence downstream is deliberate and worth stating: a column carrying Label rather than a Measure. That is the honest answer — the column is not numeric — and it is visible in the wizard, where a fabricated zero was not. +Since improvise-8jk6.4 the numeric rule is `csv_parser::number_in`, applied +through `typed` only where a number is wanted, and the parser itself yields +text. So this test asserts that `typed` leaves each token as text. The +`unwrap_or` fault would be injected into `number_in` now. + ### an_ordinary_float_is_still_a_number The control for the entry above. Guarding non-finiteness by deleting the float branch entirely would also pass that test, so this one pins that `1000.50` is still a `Number` and that `0` — the value the bug invented — is still reachable -when the file really says so. +when the file really says so. Since improvise-8jk6.4 both go through `typed`. ### a_repeated_header_is_renamed_rather_than_overwriting_its_twin @@ -1856,6 +1866,68 @@ 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_csv_column_read_as_a_category_keeps_its_text_and_as_a_measure_its_number + +improvise-8jk6.4, at the pipeline. It checks three things in one fixture, +one per reader of a CSV value: + +- analysis still proposes the numeric-looking `commit` as a Measure (typed); +- forced to Category, its items are `00565796` and `220944e2` (text); +- `lines` is stored as the number 5 (typed). + +The rivals, each run: + +- **R1**, a category named from the typed value, fails it on the items. +- **R2**, a CSV measure read with `as_f64` on its text, fails it on the cell. +- **R3**, CSV analysed without typing, fails it on the proposal. + +R2 and R3 also fail the five lpzu merge tests in this module and five +headless imports. That is how much of the suite leans on CSV measures reading +as numbers. + +This test uses `from_csv` and so cannot compile on the base. The base failure +for 8jk6.4 is the headless test +`a_csv_column_forced_to_category_keeps_its_text_verbatim`. + +### a_formula_that_does_not_parse_stops_the_build_and_is_named + +improvise-8jk6.5. On the base the build returned `Ok`, keeping `Good` and +silently dropping `Bad`. Two formulas fail to parse here, so the test proves +every failure is named. **R5**, naming only the first, fails it on `Worse`, +and since improvise-8jk6.17 the headless +`an_import_formula_that_does_not_parse_refuses_names_it_and_writes_nothing` +sees R5 too (it passes two bad formulas). It also asserts `Good` is not named, so a +message that lists every formula cannot pass. **R4** (drop, as the base did) +fails it. + +### confirming_a_formula_that_does_not_parse_keeps_it_out_and_the_text_in + +improvise-8jk6.5, the wizard half. On the base `confirm_formula` pushed +`Bad = revenue +` into the list, which failed on `pipeline.formulas`. The test +asserts the text is still in the buffer and the editor still open. A fix that +refused but cleared the buffer would throw away what the user typed, the same +hazard improvise-v4hg fixed in the TUI formula minibuffer. **R6**, confirming +regardless, fails it. The headless suite cannot see R6. + +### a_build_that_keeps_no_record_refuses_and_says_why + +improvise-8jk6.6. On the base both halves got `At least one category must be +accepted`, a refusal right only by accident. It asserts the exact message for +the two cases `nothing_to_import` distinguishes: all skipped, and no records +at all. + +- **R7**, the check removed from the build, fails it. +- **R9**, `any` swapped for `all`, also fails it, on the empty source: `all` + is vacuously true over no records. + +### the_review_step_says_when_no_record_can_be_imported + +improvise-8jk6.6, the wizard half (gotcha 22: the check lives on the step +where the user can still choose another array path). On the base the step +refused with `No categories: … Press c to cycle a field's kind`, which offers +a key that has no field to act on. **R8**, the step without the check, fails +it. The build test does not see R8. + ### 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 @@ -1943,3 +2015,34 @@ to build one `Date` category and drop the `--axis` for the vanished `Date_Month`, exit 0. It now exits 1 naming the category and `--split-date`. This also refuses any `--axis` naming an absent category, which was silently ignored before this PR as well. + +### a_folder_of_only_header_only_files_keeps_no_record_and_refuses + +improvise-8jk6.6 meeting PR #314's rule that a header-only file is still a +file. Written after merging `main` at `c91cc055`, where the two rules first +met. The file seeds give the pipeline a `File` Category proposal even with no +records. So without `nothing_to_import` the build succeeds and saves `File` +items and empty views: the category check alone cannot refuse. The `setup` +assertion proves that seeded proposal exists, so the test is not passing only +because there was no category. **R7**, the check removed from the build, fails +it. The user confirmed the refusal on PR #321: "Refuse (as #321 does)". + +### an_absent_axis_category_that_is_no_date_part_names_what_was_built + +improvise-8jk6.3 review (B2). The README's own recipe passed +`--axis Measure:column`, a silent no-op because the category is `_Measure`; the +first refusal blamed dates for it. The test uses a non-date absent name and +asserts the message lists the built categories and carries no date hint. +Fault injected: making the hint unconditional fails it. + +### the_readmes_import_recipe_runs + +improvise-8jk6.3 review (B2). Runs the README's recipe as printed. Fault +injected: restoring `--axis Measure:column` in the README fails it with the +refusal. + +### split_date_on_a_time_field_with_no_date_format_is_refused + +improvise-8jk6.3 review. `--time Revenue --split-date Revenue` made the field a +time category with no date format and the split was a silent no-op. Without +the `date_format.is_some()` condition the import exits 0 and this fails. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index a0b674dd..17a947f7 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1247,7 +1247,7 @@ pub struct ImportJsonHeadless { } impl Effect for ImportJsonHeadless { fn apply(&self, app: &mut App) { - use crate::import::analyzer::{FieldKind, analyze_records}; + use crate::import::analyzer::FieldKind; use crate::import::wizard::{DuplicatePolicy, ImportPipeline}; let is_csv = self @@ -1255,10 +1255,10 @@ impl Effect for ImportJsonHeadless { .extension() .is_some_and(|ext| ext.eq_ignore_ascii_case("csv")); - // Parse the file exactly once: `records` feeds analysis, `raw` is reused - // as the pipeline's value (improvise-oaq). CSV warnings go to the status. + // Parse the file exactly once, into the pipeline that reads it + // (improvise-oaq, improvise-8jk6.4). CSV warnings go to the status. let mut warnings: Vec = vec![]; - let (records, raw) = if is_csv { + let mut pipeline = if is_csv { let bytes = match app.storage.read(&self.path) { Ok(b) => b, Err(e) => { @@ -1269,8 +1269,7 @@ impl Effect for ImportJsonHeadless { match crate::import::csv_parser::parse_csv_bytes_with_warnings(&bytes) { Ok((recs, w)) => { warnings = w; - let raw = serde_json::Value::Array(recs.clone()); - (recs, raw) + ImportPipeline::from_csv(recs) } Err(e) => { refuse_and_stop(app, format!("CSV error: {e}")); @@ -1293,38 +1292,28 @@ impl Effect for ImportJsonHeadless { } }; 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) => { refuse_and_stop(app, msg); return; } } }; - - let proposals = analyze_records(&records); - - let pipeline = ImportPipeline { - raw, - array_paths: vec![], - selected_path: self.array_path.as_deref().unwrap_or("").to_string(), - records, - 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![], - }; + pipeline.model_name = self + .model_name + .clone() + .unwrap_or_else(|| "Imported Model".to_string()); + // Headless: nobody to ask, so a collision refuses (improvise-cgqt). + pipeline.on_duplicate = DuplicatePolicy::Refuse; + for p in &mut pipeline.proposals { + p.accepted = p.kind != FieldKind::Label; + } match pipeline.build_model_with_warnings() { Ok((new_workbook, build_warnings)) => { diff --git a/crates/improvise-io/src/import/csv_parser.rs b/crates/improvise-io/src/import/csv_parser.rs index 355b27fb..3a7cadfc 100644 --- a/crates/improvise-io/src/import/csv_parser.rs +++ b/crates/improvise-io/src/import/csv_parser.rs @@ -7,14 +7,32 @@ use serde_json::Value; 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); + +impl std::ops::Deref for CsvRecords { + type Target = [Value]; + fn deref(&self) -> &[Value] { + &self.0 + } +} + +impl CsvRecords { + pub(crate) fn into_values(self) -> Vec { + self.0 + } +} + pub fn csv_path_p(path: &Path) -> bool { path.extension() .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 -/// stderr; [`parse_csv_with_warnings`] captures them instead. -pub fn parse_csv(path: &Path) -> Result> { +/// Parse a CSV file into records. Short-row warnings go to stderr; +/// [`parse_csv_with_warnings`] captures them instead. +pub fn parse_csv(path: &Path) -> Result { let (records, warnings) = parse_csv_with_warnings(path)?; for warning in &warnings { eprintln!("warning: {warning}"); @@ -24,7 +42,7 @@ pub fn parse_csv(path: &Path) -> Result> { /// Parse a CSV file, returning records plus warnings. A short row keeps every /// header column, missing ones `Null`, each reported with a row count. -pub fn parse_csv_with_warnings(path: &Path) -> Result<(Vec, Vec)> { +pub fn parse_csv_with_warnings(path: &Path) -> Result<(CsvRecords, Vec)> { let bytes = std::fs::read(path) .with_context(|| format!("Failed to open CSV file: {}", path.display()))?; parse_csv_bytes_with_warnings(&bytes) @@ -32,9 +50,9 @@ 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)> { +pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(CsvRecords, Vec)> { let (_, records, warnings) = parse_csv_bytes(bytes)?; - Ok((records, warnings)) + Ok((CsvRecords(records), warnings)) } /// The header (after renaming), the records and the warnings. @@ -152,7 +170,7 @@ 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> { +pub fn merge_csvs(paths: &[impl AsRef]) -> Result { let merged = merge_csvs_from(&PhysicalFs, paths)?; for warning in &merged.warnings { eprintln!("warning: {warning}"); @@ -173,7 +191,7 @@ pub struct SourceFile { /// Several CSV files as one record set, with each file's own columns kept. #[derive(Debug, Clone, PartialEq)] pub struct Merged { - pub records: Vec, + pub records: CsvRecords, /// One per distinct `File` item, in the order the files were read. pub sources: Vec, /// 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). pub fn merge_csvs_from(fs: &dyn Storage, paths: &[impl AsRef]) -> Result { let mut merged = Merged { - records: Vec::new(), + records: CsvRecords::default(), sources: Vec::new(), warnings: Vec::new(), }; @@ -212,7 +230,7 @@ pub fn merge_csvs_from(fs: &dyn Storage, paths: &[impl AsRef]) -> Result Result> { 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 { - if field.is_empty() { - return Value::Null; + match field.is_empty() { + true => Value::Null, + false => Value::String(field.to_string()), } +} - // Try to parse as number (integer or float) - if let Ok(num) = field.parse::() { - return Value::Number(serde_json::Number::from(num)); +/// The number a CSV field's text reads as, where a number is wanted. +pub fn number_in(text: &str) -> Option { + if let Ok(num) = text.parse::() { + return Some(serde_json::Number::from(num)); } - // 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. - if let Ok(num) = field.parse::() - && let Some(number) = serde_json::Number::from_f64(num) - { - return Value::Number(number); - } + // measurement (improvise-jc95), so the token stays text. + text.parse::() + .ok() + .and_then(serde_json::Number::from_f64) +} - // Otherwise treat as string - Value::String(field.to_string()) +/// A CSV value as a typed JSON value: numeric text becomes its number. +pub fn typed(value: &Value) -> Value { + match value.as_str().and_then(number_in) { + Some(number) => Value::Number(number), + None => value.clone(), + } } #[cfg(test)] @@ -310,8 +335,9 @@ mod tests { assert_eq!(records.len(), 2); assert_eq!(records[0]["Region"], Value::String("East".to_string())); assert_eq!(records[0]["Product"], Value::String("Shirts".to_string())); + assert_eq!(records[0]["Revenue"], Value::String("1000".to_string())); assert_eq!( - records[0]["Revenue"], + typed(&records[0]["Revenue"]), Value::Number(serde_json::Number::from(1000)) ); } @@ -323,9 +349,10 @@ mod tests { let records = parse_csv(&path).unwrap(); 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!( - records[0]["Revenue"], + typed(&records[0]["Revenue"]), Value::Number(serde_json::Number::from_f64(1000.50).unwrap()) ); } @@ -363,10 +390,10 @@ mod tests { assert_eq!(records.len(), 2); assert_eq!(records[0]["Name"], Value::String("Widget".to_string())); assert_eq!( - records[0]["Count"], + typed(&records[0]["Count"]), 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())); } @@ -386,10 +413,7 @@ mod tests { assert_eq!(records[3]["File"], Value::String("expenses".to_string())); // Original fields preserved assert_eq!(records[0]["Region"], Value::String("East".to_string())); - assert_eq!( - records[2]["Revenue"], - Value::Number(serde_json::Number::from(50)) - ); + assert_eq!(records[2]["Revenue"], Value::String("50".to_string())); } #[test] @@ -492,10 +516,10 @@ mod tests { ] ); 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", "age-months": 2, "File": "age"}), + 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"}), ] ); } @@ -656,10 +680,7 @@ mod tests { assert!(warnings.is_empty()); assert_eq!(records.len(), 2); assert_eq!(records[0]["Region"], Value::String("East".to_string())); - assert_eq!( - records[0]["Revenue"], - Value::Number(serde_json::Number::from(1)) - ); + assert_eq!(records[0]["Revenue"], Value::String("1".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(); @@ -714,7 +735,8 @@ mod tests { token.parse::().is_ok_and(|n| !n.is_finite()), "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); } } @@ -723,10 +745,10 @@ mod tests { #[test] fn an_ordinary_float_is_still_a_number() { assert_eq!( - parse_csv_field("1000.50"), + typed(&Value::String("1000.50".into())), 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 @@ -735,9 +757,9 @@ mod tests { 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 map = records[0].as_object().unwrap(); - assert_eq!(map["A"], Value::Number(1.into())); - assert_eq!(map["B"], Value::Number(2.into())); - assert_eq!(map["A_2"], Value::Number(3.into())); + assert_eq!(map["A"], Value::String("1".into())); + assert_eq!(map["B"], Value::String("2".into())); + assert_eq!(map["A_2"], Value::String("3".into())); assert_eq!(warnings.len(), 1); assert!(warnings[0].contains("'A'") && warnings[0].contains("'A_2'")); assert!(warnings[0].contains("column 3"), "{}", warnings[0]); @@ -750,8 +772,8 @@ mod tests { let (records, _) = parse_csv_bytes_with_warnings(b"A,A_2,A\n1,2,3\n").unwrap(); let map = records[0].as_object().unwrap(); assert_eq!(map.len(), 3, "{map:?}"); - assert_eq!(map["A_2"], Value::Number(2.into())); - assert_eq!(map["A_3"], Value::Number(3.into())); + assert_eq!(map["A_2"], Value::String("2".into())); + assert_eq!(map["A_3"], Value::String("3".into())); } #[test] @@ -786,8 +808,9 @@ mod tests { assert_eq!(records.len(), 2); 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!( - records[0]["Amount"], + typed(&records[0]["Amount"]), Value::Number(serde_json::Number::from_f64(-50.00).unwrap()) ); assert_eq!(records[0]["Flag"], Value::String("*".to_string())); @@ -797,7 +820,7 @@ mod tests { Value::String("VENMO PAYMENT 260331".to_string()) ); assert_eq!( - records[1]["Amount"], + typed(&records[1]["Amount"]), Value::Number(serde_json::Number::from_f64(-240.00).unwrap()) ); } diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index 6c8cee29..5bac7ec5 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -7,8 +7,8 @@ use super::analyzer::{ DateComponent, DateShape, FieldKind, FieldProposal, analyze_records, extract_array_at_path, extract_date_component, find_array_paths, }; -use super::csv_parser::{FILE_CATEGORY, Merged, SourceFile}; -use crate::formula::parse_formula; +use super::csv_parser::{self, CsvRecords, FILE_CATEGORY, Merged, SourceFile}; +use crate::formula::{Formula, parse_formula}; use crate::model::MAX_CATEGORIES; use crate::model::cell::{CellKey, CellValue}; use crate::view::{Axis, VIRTUAL_VIEW_NAMES, View, ViewStep, records_steps}; @@ -33,6 +33,42 @@ pub struct ImportPipeline { /// 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, + /// 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 { + 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). @@ -57,7 +93,17 @@ pub enum DuplicatePolicy { } impl ImportPipeline { + /// A pipeline over a JSON value. 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 mut pipeline = Self { raw: raw.clone(), @@ -69,6 +115,7 @@ impl ImportPipeline { formulas: vec![], on_duplicate: DuplicatePolicy::Refuse, sources: vec![], + scalars, }; // Auto-select if root is an array or there is exactly one candidate path. @@ -84,7 +131,7 @@ impl ImportPipeline { /// 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)); + let mut pipeline = Self::from_csv(merged.records); pipeline.seed_sources(merged.sources); pipeline } @@ -112,7 +159,52 @@ impl ImportPipeline { self.selected_path = path.to_string(); if let Some(arr) = extract_array_at_path(&self.raw, path) { 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 = self + .records + .iter() + .map(|r| self.scalars.typed_record(r)) + .collect(); + self.proposals = analyze_records(&typed); + } + + /// The warning for the records the build will skip, if it skips any. + pub fn skip_warning(&self) -> Option { + let skipped = self.records.iter().filter(|r| !r.is_object()).count(); + skip_warnings(self.records.len(), skipped) + } + + /// Why there is nothing to build, when no record is an object to place. + pub fn nothing_to_import(&self) -> Option { + 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> { + let mut parsed = vec![]; + let mut refused: Vec = 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"))), } } @@ -244,6 +336,7 @@ impl ImportPipeline { labels: accepted(&[FieldKind::Label]), date_extractions, date_hierarchies, + scalars: self.scalars, } } @@ -254,9 +347,13 @@ impl ImportPipeline { let categories = &plan.categories; let date_extractions = &plan.date_extractions; + if let Some(why) = self.nothing_to_import() { + return Err(anyhow!(why)); + } if categories.is_empty() { return Err(anyhow!("At least one category must be accepted")); } + let formulas = self.parsed_formulas()?; let placements: Vec = self.records.iter().map(|r| plan.place(r)).collect(); if let Some(clash) = plan.spelling_clash(&placements) { @@ -313,7 +410,6 @@ impl ImportPipeline { wb.add_item("_Measure", &m.field)?; } - 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. let mut unparsed_dates = vec![0usize; date_extractions.len() + plan.date_hierarchies.len()]; @@ -329,7 +425,7 @@ impl ImportPipeline { for (at, placement) in placements.into_iter().enumerate() { match placement { - Placement::NotObject => skipped_non_objects += 1, + Placement::NotObject => {} Placement::Placed { mut coords, measures, @@ -381,22 +477,15 @@ impl ImportPipeline { wb.model.set_cell(cell.key, CellValue::Number(cell.total)); } - // Parse and add formulas - // Formulas target the "_Measure" category by default. - 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 formula in formulas { + wb.model.add_formula(formula); } 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(); + let mut warnings: Vec = self.skip_warning().into_iter().collect(); let (split, whole) = unparsed_dates.split_at(date_extractions.len()); warnings.extend(unparsed_date_warnings(date_extractions, split)); warnings.extend( @@ -508,6 +597,7 @@ struct Plan<'a> { labels: Vec<&'a FieldProposal>, date_extractions: Vec<(&'a str, &'a str, DateComponent, String)>, date_hierarchies: Vec>, + scalars: Scalars, } /// A date column imported as ONE category: its items are the finest chosen @@ -616,7 +706,7 @@ impl Plan<'_> { let measures = self .measures .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(); Placement::Placed { coords, @@ -805,6 +895,11 @@ fn unparsed_date_warnings( .collect() } +/// An import formula, targeting `_Measure`, or the error naming it (improvise-8jk6.5). +fn import_formula(raw: &str) -> Result { + 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 /// missing a category value is placed, so only a non-object is skipped. fn skip_warnings(total: usize, skipped_non_objects: usize) -> Option { @@ -992,7 +1087,9 @@ impl ImportWizard { fn validation_error(&self) -> Option { match self.step { WizardStep::ReviewProposals => self - .no_category_message() + .pipeline + .nothing_to_import() + .or_else(|| self.no_category_message()) .or_else(|| self.category_limit_message()), WizardStep::ConfigureDates => self .category_limit_message() @@ -1301,14 +1398,24 @@ impl ImportWizard { 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) { let text = self.formula_buffer.trim().to_string(); - if !text.is_empty() { - self.pipeline.formulas.push(text); + match (text.is_empty(), import_formula(&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_editing = false; + self.message = None; self.cursor = self.pipeline.formulas.len().saturating_sub(1); } @@ -2430,8 +2537,11 @@ mod tests { ), ]; - fn code_maat_records() -> Value { - Value::Array(merged_pipeline(&CODE_MAAT).records) + /// The code-maat merge without its sources, so no per-file views. + 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. @@ -2733,7 +2843,7 @@ src/a.rs,2 /// 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 p = code_maat_without_views(); let kinds: Vec<(&str, FieldKind)> = p .proposals .iter() @@ -2775,7 +2885,7 @@ src/a.rs,2 /// "". 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()); + let mut p = code_maat_without_views(); p.proposals .iter_mut() .find(|f| f.field == "coupled") @@ -3380,6 +3490,110 @@ src/a.rs,2 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 meets lpzu's header-only rule: a folder of nothing but + /// headers kept no record. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn a_folder_of_only_header_only_files_keeps_no_record_and_refuses() { + let p = merged_pipeline(&[("m/a.csv", "entity,x\n"), ("m/b.csv", "entity,y\n")]); + assert!(p.proposals.iter().any(|f| f.field == "File"), "setup"); + let msg = p.build_model().expect_err("refuses").to_string(); + assert_eq!(msg, "Nothing to import: the source has no records"); + } + + /// 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"); + } } /// improvise-8jk6.3: a date column imports as ONE hierarchical category. diff --git a/crates/improvise-io/src/persistence/roundtrip_tests.rs b/crates/improvise-io/src/persistence/roundtrip_tests.rs index 4c2c7c69..a6c84ceb 100644 --- a/crates/improvise-io/src/persistence/roundtrip_tests.rs +++ b/crates/improvise-io/src/persistence/roundtrip_tests.rs @@ -1031,7 +1031,7 @@ fn a_csv_header_with_an_embedded_newline_survives_a_save() { let csv = "\"Region\nName\",Amount\nEast,1\nWest,2\n"; let (records, _) = 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() .unwrap(); assert!( diff --git a/src/main.rs b/src/main.rs index ab94e508..947b3999 100644 --- a/src/main.rs +++ b/src/main.rs @@ -105,6 +105,10 @@ struct ImportArgs { #[arg(long, value_enum, default_value_t = OnDuplicate::Refuse)] on_duplicate: OnDuplicate, + /// With --no-wizard, refuse (and write nothing) if any record is skipped + #[arg(long)] + strict: bool, + /// Save to file instead of opening TUI #[arg(short, long)] output: Option, @@ -166,6 +170,7 @@ impl Runnable for ImportArgs { formulas: self.formula, name: self.name, on_duplicate: self.on_duplicate, + strict: self.strict, }; if self.no_wizard { @@ -201,6 +206,7 @@ struct ImportConfig { formulas: Vec, name: Option, on_duplicate: OnDuplicate, + strict: bool, } fn parse_colon_pairs(args: &[String]) -> Vec<(String, String)> { @@ -305,6 +311,23 @@ fn apply_axis_overrides(wb: &mut Workbook, axes: &[(String, String)]) { } } +/// Why `--axis` is refused: the categories the import did build, and a date +/// hint only for a name shaped like one of the split date parts. +fn axis_refusal(cat: &str, built: &[String]) -> String { + let date_part = ["_Year", "_Month", "_Quarter"] + .iter() + .any(|suffix| cat.ends_with(suffix)); + let hint = if date_part { + " A date's parts are one category unless --split-date names it." + } else { + "" + }; + format!( + "--axis names '{cat}', which this import did not build.{hint} Built: {}", + built.join(", ") + ) +} + fn run_headless_import( mut pipeline: ImportPipeline, config: &ImportConfig, @@ -312,11 +335,16 @@ fn run_headless_import( model_file: Option, ) -> Result<()> { apply_config_to_pipeline(&mut pipeline, config); + // A strict skip refuses before the build, so nothing is written (8jk6.6). + if let (true, Some(skipped)) = (config.strict, pipeline.skip_warning()) { + anyhow::bail!("--strict: {skipped}"); + } if let Some(field) = config.split_dates.iter().find(|f| { - !pipeline - .proposals - .iter() - .any(|p| &p.field == *f && p.kind == import::analyzer::FieldKind::TimeCategory) + !pipeline.proposals.iter().any(|p| { + &p.field == *f + && p.kind == import::analyzer::FieldKind::TimeCategory + && p.date_format.is_some() + }) }) { anyhow::bail!("--split-date '{field}' is not a date column of this import"); } @@ -335,10 +363,10 @@ fn run_headless_import( .iter() .find(|(cat, _)| wb.model.category(cat).is_none()) { - anyhow::bail!( - "--axis names '{cat}', which this import did not build \ - (a date's parts are one category unless --split-date names it)" - ); + anyhow::bail!(axis_refusal( + cat, + &wb.model.categories.keys().cloned().collect::>() + )); } apply_axis_overrides(&mut wb, &config.axes); @@ -416,7 +444,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(ImportPipeline::new(Value::Array(records))), + Ok(records) => Some(ImportPipeline::from_csv(records)), Err(e) => { eprintln!("CSV parse error: {e}"); None diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index 68021682..2475931d 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -64,8 +64,8 @@ fn duplicate_csv_headers_not_silent() { 2, "both columns must survive as separate fields: {fields:?}" ); - assert_eq!(fields["A"].as_f64(), Some(1.0), "{fields:?}"); - assert_eq!(fields["A_2"].as_f64(), Some(2.0), "{fields:?}"); + assert_eq!(fields["A"], "1", "{fields:?}"); + assert_eq!(fields["A_2"], "2", "{fields:?}"); assert!( warnings .iter() @@ -78,7 +78,7 @@ fn duplicate_csv_headers_not_silent() { fn blank_measure_still_infers_numeric() { use improvise_io::import::{analyzer::FieldKind, wizard::ImportPipeline}; 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(); assert_eq!(proposal.kind, FieldKind::Measure); } @@ -87,7 +87,7 @@ fn repeated_import_coordinates_not_silently_overwritten() { use improvise_io::import::wizard::ImportPipeline; let (records, _) = 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; let (wb, warnings) = pipeline.build_model_with_warnings().unwrap(); let values: Vec = wb @@ -400,7 +400,7 @@ fn ordinary_controls() { let (records, warnings) = parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nWest,20\n").unwrap(); assert!(warnings.is_empty()); - let p = ImportPipeline::new(serde_json::Value::Array(records)); + let p = ImportPipeline::from_csv(records); assert_eq!( p.proposals .iter() diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index 572740a2..1e0e11f4 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -703,6 +703,77 @@ fn an_axis_naming_a_category_the_import_did_not_build_is_refused() { ); } +/// `--time` on a column with no date format makes it a time category without +/// dates, so there is nothing to split. +#[test] +fn split_date_on_a_time_field_with_no_date_format_is_refused() { + let scratch = Scratch::new("split_date_no_format"); + let out_path = scratch.path("out.improv"); + let out = run_import( + &dated_csv(&scratch), + &out_path, + &["--time", "Revenue", "--split-date", "Revenue"], + ); + assert!(!out.status.success(), "{}", stderr_of(&out)); + assert!( + stderr_of(&out).contains("--split-date"), + "{}", + stderr_of(&out) + ); +} + +/// A name that is no date part gets the categories that exist, never the date +/// hint (the README's old `--axis Measure:column` was this). +#[test] +fn an_absent_axis_category_that_is_no_date_part_names_what_was_built() { + let scratch = Scratch::new("axis_unknown_plain"); + let out_path = scratch.path("out.improv"); + let out = run_import( + &dated_csv(&scratch), + &out_path, + &["--axis", "Measure:column"], + ); + assert!(!out.status.success(), "{}", stderr_of(&out)); + let err = stderr_of(&out); + assert!( + err.contains("'Measure'") && err.contains("_Measure"), + "{err}" + ); + assert!(!err.contains("--split-date"), "no date hint: {err}"); +} + +/// The README's import recipe runs as printed. +#[test] +fn the_readmes_import_recipe_runs() { + let root = Path::new(env!("CARGO_MANIFEST_DIR")); + let readme = std::fs::read_to_string(root.join("README.md")).unwrap(); + let start = readme.find("improvise import examples/demo.csv").unwrap(); + let recipe = readme[start..] + .split("```") + .next() + .unwrap() + .replace("\\\n", " "); + let mut args: Vec = vec![]; + for (i, part) in recipe.split('"').enumerate() { + match i % 2 { + 1 => args.push(part.to_string()), + _ => args.extend(part.split_whitespace().map(String::from)), + } + } + let at = args.iter().position(|a| a == "-o").unwrap(); + args.truncate(at); + let scratch = Scratch::new("readme_recipe"); + let out = Command::new(bin()) + .arg("import") + .arg(root.join("examples/demo.csv")) + .args(args.iter().skip(3)) + .arg("-o") + .arg(scratch.path("out.improv")) + .output() + .unwrap(); + assert!(out.status.success(), "{}", stderr_of(&out)); +} + /// 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"); @@ -799,6 +870,192 @@ fn a_headless_import_takes_files_and_folders_together_and_refuses_an_empty_folde assert!(!scratch.path("never.improv").exists()); } +/// Runs `import --no-wizard -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", + "--formula", + "W = * 2", + ], + ); + 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("'W = * 2'"), "names every bad one: {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}"); +} + +/// A JSON array with one stray scalar among its records. +const ONE_STRAY: &str = r#"[{"a": "k", "x": 1}, 2, {"a": "m", "x": 4}]"#; + +/// improvise-8jk6.6, the user's ruling: `--strict` makes any skip a refusal +/// that writes nothing. Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_strict_import_that_skips_a_record_refuses_and_writes_nothing() { + let scratch = Scratch::new("strict_skip"); + let (out, out_path) = import_of(&scratch, "p.json", ONE_STRAY, &["--strict"]); + let err = stderr_of(&out); + assert!(!out.status.success(), "{err}"); + assert!( + err.contains("--strict: Skipped 1 of 3 records that were not JSON objects"), + "{err}" + ); + assert!(!out_path.exists(), "a strict refusal writes nothing"); +} + +/// The control: `--strict` refuses a skip, not every import. +#[test] +fn a_strict_import_that_skips_nothing_still_writes() { + let scratch = Scratch::new("strict_clean"); + let body = r#"[{"a": "k", "x": 1}, {"a": "m", "x": 4}]"#; + let (out, out_path) = import_of(&scratch, "p.json", body, &["--strict"]); + assert!(out.status.success(), "{}", stderr_of(&out)); + let doc = std::fs::read_to_string(&out_path).unwrap(); + assert!(doc.contains("_Measure=x, a=m = 4"), "{doc}"); +} + +/// improvise-8jk6.17 (F3): a CSV decimal measure is its number, not dropped +/// text. Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_csv_decimal_measure_imports_as_its_number() { + let scratch = Scratch::new("csv_decimal"); + let body = "Region,Revenue\nEast,1000.50\nWest,-2.25\n"; + let (out, out_path) = import_of(&scratch, "f.csv", body, &[]); + assert!(out.status.success(), "{}", stderr_of(&out)); + let doc = std::fs::read_to_string(&out_path).unwrap(); + for line in [ + "Region=East, _Measure=Revenue = 1000.5", + "Region=West, _Measure=Revenue = -2.25", + ] { + assert!(doc.lines().any(|l| l == line), "{line} missing from\n{doc}"); + } +} + +/// improvise-8jk6.17 (F1): `import-json` reads a CSV as CSV, so its numeric +/// column is a measure. Evidence: `context/test-evidence/cli.md`. +#[test] +fn import_json_on_a_csv_reads_its_numeric_column_as_a_measure() { + let scratch = Scratch::new("import_json_csv"); + let csv = scratch.path("f.csv"); + std::fs::write(&csv, "Region,Revenue\nEast,1000.50\nWest,7\n").unwrap(); + let doc = scratch.path("m.improv"); + let out = run_cmd(&[&format!("import-json {}", csv.display())], &doc); + assert!(out.status.success(), "{}", stderr_of(&out)); + let text = std::fs::read_to_string(&doc).unwrap(); + assert!(!text.contains("## Category: Revenue"), "{text}"); + for line in [ + "Region=East, _Measure=Revenue = 1000.5", + "Region=West, _Measure=Revenue = 7", + ] { + assert!( + text.lines().any(|l| l == line), + "{line} missing from\n{text}" + ); + } +} + /// 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] diff --git a/tests/records_wide_smoke.rs b/tests/records_wide_smoke.rs index c121b25a..38cfbbe3 100644 --- a/tests/records_wide_smoke.rs +++ b/tests/records_wide_smoke.rs @@ -18,7 +18,7 @@ fn imported_model_shows_one_row_per_record_with_measure_columns() { .expect("write csv"); 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 saved = dir.path().join("w.improv");