fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge remote-tracking branch 'gh/main' into feat/8jk6.3-date-hierarchy

Browse Source 
# Conflicts:
#	context/test-evidence/formula-and-io.md
#	crates/improvise-io/src/import/wizard.rs
#	src/main.rs
This commit is contained in:
Claude (8jk63-fix)
2026-10-02 22:00:32 -07:00
committed by Edward Langley
parent 0c56a4b9a1 7e87970278
commit 581934886d
13 changed files with 974 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files
README.md
+1 -1
View File
@@ -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" \
context/code/cli.md
+33
View File
@@ -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.
context/code/formula-and-io.md
+112
View File
@@ -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<Value>`.** 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<Value>`, 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 <folder>` exists for improvise-1xfb.5.1. code-maat writes one
context/test-evidence/cli.md
+90
View File
@@ -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.
context/test-evidence/formula-and-io.md
+104 -1
View File
@@ -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.
crates/improvise-command/src/ui/effect.rs
+19 -30
View File
@@ -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<String> = 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
pipeline.model_name = self
.model_name
.as_deref()
.unwrap_or("Imported Model")
.to_string(),
formulas: vec![],
.clone()
.unwrap_or_else(|| "Imported Model".to_string());
// Headless: nobody to ask, so a collision refuses (improvise-cgqt).
on_duplicate: DuplicatePolicy::Refuse,
sources: vec![],
};
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)) => {
crates/improvise-io/src/import/csv_parser.rs
+72 -49
View File
@@ -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<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 {
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<Vec<Value>> {
/// 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<CsvRecords> {
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<Vec<Value>> {
/// 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<Value>, Vec<String>)> {
pub fn parse_csv_with_warnings(path: &Path) -> Result<(CsvRecords, Vec<String>)> {
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<Value>, Vec<String>)>
/// 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<Value>, Vec<String>)> {
pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(CsvRecords, Vec<String>)> {
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<String>) -> (Vec<String>, Vec<String>) {
/// 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<Path>]) -> Result<Vec<Value>> {
pub fn merge_csvs(paths: &[impl AsRef<Path>]) -> Result<CsvRecords> {
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<Value>,
pub records: CsvRecords,
/// One per distinct `File` item, in the order the files were read.
pub sources: Vec<SourceFile>,
/// 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<Path>]) -> Result<Merged> {
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<Path>]) -> Result<M
if let Value::Object(ref mut map) = record {
map.insert(FILE_CATEGORY.to_string(), Value::String(stem.clone()));
}
merged.records.push(record);
merged.records.0.push(record);
}
}
Ok(merged)
@@ -266,26 +284,33 @@ fn csv_documents_in(fs: &dyn Storage, dir: &Path) -> Result<Vec<PathBuf>> {
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::<i64>() {
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<serde_json::Number> {
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
// measurement (improvise-jc95), so the token falls through and stays text.
if let Ok(num) = field.parse::<f64>()
&& let Some(number) = serde_json::Number::from_f64(num)
{
return Value::Number(number);
// measurement (improvise-jc95), so the token stays text.
text.parse::<f64>()
.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::<f64>().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())
);
}
crates/improvise-io/src/import/wizard.rs
+237 -23
View File
@@ -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<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).
@@ -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<Value> = 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<String> {
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<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"))),
}
}
@@ -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<Placement> = 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) {
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<String> = skip_warnings(self.records.len(), skipped_non_objects)
.into_iter()
.collect();
let mut warnings: Vec<String> = 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<DateHierarchy<'a>>,
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<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
/// missing a category value is placed, so only a non-object is skipped.
fn skip_warnings(total: usize, skipped_non_objects: usize) -> Option<String> {
@@ -992,7 +1087,9 @@ impl ImportWizard {
fn validation_error(&self) -> Option<String> {
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() {
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.
crates/improvise-io/src/persistence/roundtrip_tests.rs
+1 -1
View File
@@ -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!(
src/main.rs
+37 -9
View File
@@ -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<PathBuf>,
@@ -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<String>,
name: Option<String>,
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<PathBuf>,
) -> 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::<Vec<_>>()
));
}
apply_axis_overrides(&mut wb, &config.axes);
@@ -416,7 +444,7 @@ fn get_import_data(args: &[PathBuf]) -> Option<ImportPipeline> {
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
tests/bug_hunt_reproductions.rs
+5 -5
View File
@@ -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<f64> = 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()
tests/headless_cli.rs
+257
View File
@@ -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<String> = 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 <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",
"--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]
tests/records_wide_smoke.rs
+1 -1
View File
@@ -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");