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