fix(io): CSV export writes the rows the view prunes (improvise-iot6)
`render_csv` built its layout with `GridLayout::new`, which does not apply `prune_empty` — only `with_frozen_records` does. So `:export` in the TUI and the CLI export path both wrote rows the grid does not draw. User-visible, in production, and shipping for as long as the two constructors have differed. The fourth instance of this trap; the third was `project::grid_in` in PR #78, and this is that fix line for line. The fixture is the difficulty, and the ticket says so. The sibling property written for improvise-t6db passed on the buggy tree in its first version, because it made its two views differ by turning pruning *off* — vacuous, since with nothing to prune both code paths agree. So this one contains a row that is actually empty, leaves `prune_empty` ON, and asserts the precondition in both directions before exporting: `with_frozen_records` draws 2 rows where `new` holds 3. If pruning ever becomes a no-op the precondition fails and says so, rather than the assertion passing vacuously. No fault needed injecting — the bug was live. On the unfixed tree the test fails with `["Food", "Gas", "Rent"]` against `["Food", "Gas"]`. None of `export.rs`'s three pre-existing tests mentions pruning at all, which is exactly why this shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -731,3 +731,34 @@ Case counts went 500 → 256 in the same file, which is the default
|
||||
a further 0.39s → 0.21s but it is not the fix, and reporting it as one would
|
||||
be the mistake this entry exists to record. Net: 1.42s → 0.21s, and
|
||||
`improvise-io` is no longer the run's longest binary.
|
||||
|
||||
### export_csv_omits_the_rows_the_view_prunes
|
||||
|
||||
improvise-iot6. `render_csv` built its layout with `GridLayout::new`, which does
|
||||
**not** apply `prune_empty` — only `GridLayout::with_frozen_records` does. So
|
||||
`:export` in the TUI and the CLI export path both wrote rows the grid does not
|
||||
draw. User-visible, in production, and shipping for as long as the two
|
||||
constructors have differed.
|
||||
|
||||
This is the **fourth** instance of the same trap; the third was
|
||||
`project::grid_in`, fixed in PR #78, and this fix is that one line for line.
|
||||
No fault needed injecting: on the unfixed tree the test fails with
|
||||
|
||||
> CSV wrote a row the grid does not draw: left `["Food", "Gas", "Rent"]`,
|
||||
> right `["Food", "Gas"]`
|
||||
|
||||
**Why the fixture is discriminating, which is the whole difficulty here.** The
|
||||
sibling property written for improvise-t6db passed on the buggy tree in its
|
||||
first version, because it made its two views differ by turning pruning *off* —
|
||||
plenty of differing config, and vacuous, since with nothing to prune both code
|
||||
paths agree. So this fixture does two things that one cannot fake:
|
||||
|
||||
- it contains a row that is **actually empty** (`Rent`, an item of `Type` that
|
||||
no cell names) and leaves `prune_empty` **ON**;
|
||||
- it asserts the precondition in both directions before exporting anything —
|
||||
`with_frozen_records` draws 2 rows and `new` holds 3. If a future change makes
|
||||
pruning a no-op, or makes the two constructors agree, the precondition fails
|
||||
and says so rather than the assertion passing vacuously.
|
||||
|
||||
None of `export.rs`'s three pre-existing tests mentions pruning at all, which is
|
||||
exactly why this shipped.
|
||||
|
||||
@@ -30,7 +30,9 @@ fn render_csv(workbook: &Workbook, view_name: &str) -> Result<Vec<u8>> {
|
||||
.get(view_name)
|
||||
.ok_or_else(|| anyhow::anyhow!("View '{view_name}' not found"))?;
|
||||
|
||||
let layout = GridLayout::new(&workbook.model, view);
|
||||
// `with_frozen_records`, not `new`: `new` skips `prune_empty`, so an export
|
||||
// writes rows the grid does not draw (improvise-iot6).
|
||||
let layout = GridLayout::with_frozen_records(&workbook.model, view, None);
|
||||
let model = &workbook.model;
|
||||
let (fmt_comma, fmt_decimals) = crate::format::parse_number_format(&view.number_format);
|
||||
|
||||
@@ -144,6 +146,63 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Bug improvise-iot6: `render_csv` built its layout with `GridLayout::new`,
|
||||
/// which never applies `prune_empty`. Evidence: `context/test-evidence/formula-and-io.md`.
|
||||
#[test]
|
||||
fn export_csv_omits_the_rows_the_view_prunes() {
|
||||
use crate::view::{Axis, GridLayout};
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Type").unwrap();
|
||||
wb.add_category("Month").unwrap();
|
||||
for item in ["Food", "Gas", "Rent"] {
|
||||
wb.add_item("Type", item).unwrap();
|
||||
}
|
||||
wb.add_item("Month", "Jan").unwrap();
|
||||
// `Rent` is the empty row: every other Type carries a Jan value.
|
||||
for t in ["Food", "Gas"] {
|
||||
wb.model.set_cell(
|
||||
coord(&[("Type", t), ("Month", "Jan")]),
|
||||
CellValue::Number(10.0),
|
||||
);
|
||||
}
|
||||
{
|
||||
let v = wb.active_view_mut();
|
||||
v.set_axis("Type", Axis::Row);
|
||||
v.set_axis("Month", Axis::Column);
|
||||
v.prune_empty = true;
|
||||
}
|
||||
|
||||
// The precondition, or this test proves nothing: pruning must really be
|
||||
// ON and must really drop a row the model still holds an item for.
|
||||
assert!(wb.active_view().prune_empty, "pruning must stay ON");
|
||||
let drawn = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None);
|
||||
assert_eq!(
|
||||
drawn.row_count(),
|
||||
2,
|
||||
"the view must draw two of the three rows"
|
||||
);
|
||||
assert_eq!(
|
||||
GridLayout::new(&wb.model, wb.active_view()).row_count(),
|
||||
3,
|
||||
"the unpruned layout must really hold the third row"
|
||||
);
|
||||
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let path = dir.path().join("export.csv");
|
||||
super::export_csv(&wb, "Default", &path).unwrap();
|
||||
let mut rdr = csv::ReaderBuilder::new()
|
||||
.has_headers(false)
|
||||
.from_path(&path)
|
||||
.unwrap();
|
||||
let records: Vec<csv::StringRecord> = rdr.records().collect::<Result<_, _>>().unwrap();
|
||||
let labels: Vec<&str> = records.iter().skip(1).map(|r| &r[0]).collect();
|
||||
assert_eq!(
|
||||
labels,
|
||||
["Food", "Gas"],
|
||||
"CSV wrote a row the grid does not draw"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn export_csv_unknown_view_returns_error() {
|
||||
let m = two_cat_model();
|
||||
|
||||
Reference in New Issue
Block a user