From d00cea2477b9b240a274c466987ed9f2738a27e1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 23:33:52 -0700 Subject: [PATCH 1/3] fix: show decimals by default, and let a formula name its own format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-e0ex. `Margin = Profit / Revenue` drew as `1`: `View::new` seeded `,.0` — zero decimals — so a 0.66 ratio rounded half-away-from-zero. The cause is the format, not the arithmetic; exact decimal math would not change the rendering by one character, so nothing here touches the evaluator and gotcha 8 is unchanged. Two halves, per the user's ruling: * `View::new` seeds `,.2`. Revenue draws `18,400.00` and Margin `0.66`. The noise on whole numbers is the cheaper error — a currency column with two redundant zeroes is legible, a ratio with none is wrong — and a format cannot be guessed from a value. * A formula may carry its own format, which wins for its target; a raw measure keeps the view's. `formula.pest` gains `format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }`, with `sp+` for `WHERE`'s reason (a predicate is invisible to the grammar-walking generator) and `FORMAT` added to `quote::KEYWORDS`. `improv.pest` is deliberately unchanged: `formula_raw` already carries the `## Formulas` line verbatim, so the format rides in the formula text with nothing to add — and the generator, which reads `improv.pest`, sees no new spelling to violate. Resolution has one home. `FormatPlan::of(model, view)` replaces the `(comma, decimals)` pair every render path threaded — `GridLayout::display_text`, `viewport::grid_body`/`compute_col_widths`, `project::grid_in` and `persistence/export.rs::render_csv` — rather than being added beside it, which would be two answers to "what format is this cell". It is asked of the cell's `CellKey`, so a cell aggregating over `_Measure` keeps the view's format by construction. `GridBody::col_formats` carries the answer to the totals strip, which is formatted on its own path. Persistence is untouched except for metadata: `format_f64` is display, `persistence/format.rs::format_number` writes round-trip precision, and the two have no call between them. Newly created views write `format: ,.2`; existing documents are unaffected, because `format_md` has always written the line explicitly. `web/demo.improv` moves to `,.2` on both views. The trap this sets is gotcha 34's: `format_md` writes `Formula::raw`, so a `FORMAT` suffix round-trips whether or not the walker parsed it. The round-trip test therefore asserts the parsed field — injecting a walker that drops the clause fails that one test and nothing else in `improvise-io`, 242 tests including the whole characterization suite and the grammar properties. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- benches/grid_projection.rs | 7 +- context/code/core.md | 118 +++++++++++++++ context/test-evidence/command.md | 28 ++++ context/test-evidence/core.md | 45 ++++++ context/test-evidence/formula-and-io.md | 33 ++++ .../src/command/cmd/panel.rs | 1 + crates/improvise-command/src/ui/app.rs | 11 +- crates/improvise-command/src/ui/host.rs | 4 +- crates/improvise-command/src/ui/project.rs | 91 +++++++---- crates/improvise-command/src/ui/viewport.rs | 47 +++--- crates/improvise-core/src/format.rs | 142 +++++++++++++++++- crates/improvise-core/src/view/layout.rs | 15 +- crates/improvise-core/src/view/types.rs | 4 +- crates/improvise-formula/src/ast.rs | 12 ++ crates/improvise-formula/src/formula.pest | 7 +- crates/improvise-formula/src/parser.rs | 13 ++ crates/improvise-formula/src/quote.rs | 4 +- crates/improvise-gp/src/dataset.rs | 9 +- crates/improvise-io/src/persistence/export.rs | 7 +- .../src/persistence/roundtrip_tests.rs | 37 +++++ .../tests/browser_round_trip.rs | 4 +- crates/improvise-worker-server/src/worker.rs | 4 +- .../tests/post_message.rs | 14 +- crates/improvise-ws-server/tests/socket.rs | 2 +- harness/tests/contract.spec.js | 6 +- harness/tests/shell.spec.js | 4 +- src/ui/grid.rs | 3 + tests/bug_hunt_reproductions.rs | 3 +- tests/web_demo_model.rs | 5 +- web/demo.improv | 4 +- 30 files changed, 579 insertions(+), 105 deletions(-) diff --git a/benches/grid_projection.rs b/benches/grid_projection.rs index 8afca583..17699856 100644 --- a/benches/grid_projection.rs +++ b/benches/grid_projection.rs @@ -3,7 +3,7 @@ use std::time::{Duration, Instant}; -use improvise::format::parse_number_format; +use improvise::format::FormatPlan; use improvise::model::cell::{CellKey, CellValue}; use improvise::view::Axis; use improvise::workbook::Workbook; @@ -94,12 +94,11 @@ fn time(iters: u32, app: &App, viewport: Viewport) -> Duration { fn time_widths(iters: u32, app: &App) -> Duration { let layout = &app.layout; let model = &app.model_state.workbook.model; - let (comma, decimals) = - parse_number_format(&app.model_state.workbook.active_view().number_format); + let plan = FormatPlan::of(model, app.model_state.workbook.active_view()); let mut best = Duration::MAX; for _ in 0..iters.max(3) { let start = Instant::now(); - std::hint::black_box(compute_col_widths(model, layout, comma, decimals)); + std::hint::black_box(compute_col_widths(model, layout, &plan)); best = best.min(start.elapsed()); } best diff --git a/context/code/core.md b/context/code/core.md index 8a043590..37e406fe 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -820,3 +820,121 @@ renders a cell and asserts its digits. That gap is named rather than closed — those files are `improvise-command`, which was another wave's ground when this landed — and it is the more useful half of this entry, because it says where the next regression would go unseen. + +**That gap is now closed, and the sentence above is kept as the record of it.** +`ui::project::cell_identity::a_formulas_format_reaches_its_own_column_and_no_other` +renders cells *and* the totals strip and asserts their digits (improvise-e0ex); +`ui/app.rs`'s own call is still unguarded, and now reaches the same `FormatPlan` +rather than a second `parse_number_format`, so there is one fewer place to drift. + +## The decimal default and a formula's own format (improvise-e0ex) + +`View::new` seeds `,.2`, not `,.0`. The symptom that forced it was +`Margin = Profit / Revenue` drawing as `1`: a 0.66 ratio under *zero* decimals, +rounded half-away-from-zero by `format_f64`. Three things worth stating, because +each was a candidate answer that is wrong. + +**It is a formatting bug and not an arithmetic one.** Exact decimal arithmetic +(ADR 005) would not change the rendering by one character — `0.66` under `,.0` +is still `1` — so nothing here waits on it and nothing here touches the +evaluator. Gotcha 8 is unchanged: `format_f64` is called in rendering and +nowhere else, and full `f64` is what eval sees. + +**The cost is paid on whole numbers, knowingly.** Revenue now draws +`18,400.00`. That is what an accounting application does, and it is the cheaper +error: a currency column with two redundant zeroes is *legible*, where a ratio +with none is **wrong**. A format cannot be guessed from a value — 0.66 is a +ratio and 0.66 is also two-thirds of a dollar — so the default must be the one +whose failure mode is noise rather than a false number. + +**The override is spelled in the formula, not per `_Measure` item.** The user's +ruling ("unless the format is explicit in the formula") picks the narrower of +two nearly-coincident units: a formula *target* is a `_Measure` item (gotcha 7), +so per-formula and per-measure differ only for a measure no formula owns — and +such a measure is raw data, whose format is the view's job. So a formula may +carry its own format, which wins for its target; a raw measure keeps the view's. + +### Where the format is resolved: `FormatPlan` + +`FormatPlan::of(model, view)` is the one answer, and every render path takes it +where it used to take a `(comma, decimals)` pair: `GridLayout::display_text`, +`ui::viewport::grid_body`/`compute_col_widths`, `ui::project::grid_in` and +`persistence/export.rs::render_csv`. Replacing the pair rather than adding a +second argument beside it is the `row_levels`-next-to-`row_labels` rule from +design-principles §5 — two answers to "what format is this cell" is the disease. + +**It is asked of the cell's `CellKey`, never of the axes.** A cell aggregating +over `_Measure` carries no target coordinate and so keeps the view's format, +which falls out of the key rather than needing a rule. Deriving it from "is +`_Measure` on a visible axis" would be a second derivation that could agree +with itself while disagreeing with the cells. + +**The empty plan short-circuits**, so a document with no formula format pays one +`is_empty` per cell and nothing else; the lookup is otherwise one hash get per +`(category, item)` pair in the plan, of which there is normally one. + +**A column total takes the format of its own column's first row.** `GridBody` +carries `col_formats` beside `col_widths` so the totals strip and the cells +above it cannot disagree. Where `_Measure` is on the *row* axis a column is not +a measure at all, so no formula format applies to any cell in it and the first +row's answer is the view's — the rule only has to be decided for the case it +cannot arise in. + +### The grammar: `FORMAT ` in the formula, not in the `.improv` line + +`formula.pest` gains `format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }` after +the optional `WHERE`. **`improv.pest` is unchanged**, and that is the design +rather than an omission: `formula_raw` already takes the `## Formulas` line +verbatim up to the optional `[Category]` suffix, so a format inside the formula +*text* is carried by the existing grammar with nothing to add. Two consequences. +The grammar-walking generator (which reads `improv.pest`, not `formula.pest`) is +untouched, so there is no new spelling for it to violate. And the same text a +user types into the TUI formula minibuffer is the text the file carries — one +namespace, one spelling rule, exactly as gotcha 34 says of names. + +`sp+` rather than a `!bare_char` lookahead, for `WHERE`'s reason (gotcha 34): +a predicate is invisible to a generator. `FORMAT` joins `quote::KEYWORDS`, so an +item literally named `FORMAT` is written `|FORMAT|` — the keyword guard is +canonical and this is one more entry in it, not a new rule. + +**`format_spec` is `(ASCII_ALPHANUMERIC | "," | "." | "%")+`** — a bounded token +where the view's `format:` line is `rest_of_line`. The line can afford to be +opaque because a newline ends it; a formula clause cannot, because something may +follow. The set covers every spelling `parse_number_format` can read (`,.2f`, +`.4`, `%.2f`) and nothing the tokenizer would have to disambiguate. + +### The trap this feature sets, and what is asserted instead + +`format_md` writes `Formula::raw`, so a format inside the raw text round-trips +**whether or not the walker parsed it into a field**. That is gotcha 34's bug +exactly — a target that kept its pipes survived every round-trip test because +`raw` was unaffected. So the round-trip test asserts `Formula::format`, the +parsed field, and its `raw` assertion is there to *show* that `raw` cannot be +the witness. Injecting the fault (a walker that drops the clause) fails that one +test and **nothing else in `improvise-io`** — 242 other tests, including the +whole `format_md`↔`parse_md` characterization suite and the grammar-walking +properties, pass. That is the measurement, not an argument. + +### What persistence does and does not do here + +The user's constraint — "should not affect persistence at all, except to store +format metadata" — holds by construction and the job was to not break it. +`format_f64` is display; `persistence/format.rs::format_number` is a different +function in a different crate writing Rust's shortest round-trip form. **The +regression to watch for is routing a save through the display formatter** so the +file matches what the user sees: a model has many views (gotcha 29), so there is +no single right answer and the last save would win. Gotcha 8 forbids feeding a +rounded value back into *eval*; this is its persistence twin. + +What did change on disk is metadata only: newly created views write +`format: ,.2` where they wrote `format: ,.0`, and a formula line may now carry +a `FORMAT` suffix. Existing documents are unaffected — `format_md` has always +written the `format:` line explicitly whenever `number_format` is non-empty and +`View::new`'s value has never been empty, so every document improvise has ever +written carries its own and no migration exists to get wrong. + +One consumer that is display but reads like persistence: **CSV export** goes +through `display_text` and therefore through the view's format, as it always +has. A document with no explicit `format:` line now exports two decimals. That +is pre-existing behaviour meeting a new default, not a new coupling. + diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 85dbc688..ec2d4875 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4216,3 +4216,31 @@ fixture loads **both** stacks first so a lost forward stack is visible at all. The neighbouring test `a_refused_switch_corrects_the_asker_and_nobody_else` compared `active_view` alone and stayed green through all of this: the check that answers a neighbouring question, in the place the doctrine warns about. + +--- + +## The format a projected cell is drawn with (improvise-e0ex) + +### a_formulas_format_reaches_its_own_column_and_no_other + +`context/code/core.md` recorded, as the more useful half of improvise-711n's +entry, that `ui/project.rs::grid_of` "has no test that renders a cell and +asserts its digits". This is that test, and it is why the gap was worth naming: +the per-formula format is resolved in `improvise-core` and could have been +resolved correctly there while never reaching the grid. + +It asserts four things that fail independently: the formula column's cell, the +raw column's cell beside it, and **both** totals. The totals matter separately +because they are formatted on their own path — `GridBody::col_formats`, read off +the top row — so a correct cell says nothing about the strip under it. + +Two faults injected: + +- `FormatPlan::at` returning the view's format for every key fails the cell + assertion (`left: Some("0.50") right: Some("0.5000")`). +- Dropping only the `col_formats` write in `viewport::grid_body`, leaving the + cells correct, fails the totals assertion (`left: "1.50" right: "1.5000"`). + +The second is the one that earns the test its length: without it, the totals +strip could round every formula column to the view's format and no assertion in +the workspace would move. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index c22115a8..23416970 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1062,3 +1062,48 @@ qualification. It asserts `effective_item_names("_Measure")` **by value** rather than `!contains("Profit")`, so a fix that removed the formula and also lost `Revenue` or `Cost` on the way fails here rather than passing a negative. + +--- + +## `format.rs` — the decimal default and a formula's own format (improvise-e0ex) + +### a_ratio_keeps_its_decimals_under_a_fresh_views_format + +The bug the ticket reports, at the smallest place it is visible. +`View::new`'s `number_format` was `,.0` — *zero* decimals — so `2/3` reached +`format_f64` and came back as `"1"`, half-away-from-zero. Written before the +fix and observed failing with `left: "1" right: "0.67"`. + +It asks the **view's default** rather than a literal `",.2"`, so the assertion +is about the shipped default and not about `parse_number_format`, which already +had its own tests. Reverting `View::new` to `,.0` fails it. + +What it deliberately does not assert: that the default is exactly two places. +`0.67` would also fail under `.1` or `.3`, which is the point — the test names +"a ratio keeps its decimals", and the precise number is a product decision +recorded in `context/code/core.md`, not a property. + +### a_formulas_own_format_wins_for_its_target_and_nowhere_else + +The override, and both halves of "nowhere else" — a raw measure beside the +formula target, and a key with no `_Measure` coordinate at all (the aggregating +cell). The fixture uses `.4` against a `,.2` view so the two answers differ in +*digits*, not merely in grouping. + +Two faults injected, in opposite directions, because one alone is not enough: + +- `FormatPlan::at` returning `self.view` unconditionally (the override never + applies) fails with `left: "0.67" right: "0.6667"`. +- `FormatPlan::at` returning the first entry in the plan for any key (the + override leaks to every cell) fails with `left: "0.6667" right: "0.67"`. + +A test asserting only the target's own column passes the second fault, which is +why the two neighbours are in the same test rather than in a sibling that could +be deleted on its own. + +### a_formula_naming_no_format_leaves_its_target_on_the_views + +The fallback, asserted where it is easiest to lose: a formula target whose +formula carries no format must be indistinguishable from a raw measure. It +keeps `FormatPlan::of` from indexing every target with some default and then +"overriding" it back, which would be a second copy of the view's answer. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index ab7581cd..6043bff6 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1255,3 +1255,36 @@ well as membership — insertion order is load-bearing here (gotcha 18), and the neighbouring `two_sections_naming_one_category_round_trip_to_a_stable_group_order` already holds groups to that standard. This test was the item-list half of the same invariant, held to a weaker one. + +--- + +## A formula's own number format on disk (improvise-e0ex) + +### parse_md_round_trips_the_format_a_formula_carries + +`format_md` writes `Formula::raw`, so a `FORMAT .4` suffix inside the formula +text survives a save/load **whether or not the tree-walker parsed it into +`Formula::format`**. That is gotcha 34's bug wearing new clothes: a target that +kept its pipe delimiters round-tripped perfectly and still created a ghost +`_Measure` item, precisely because `raw` was unaffected. + +So this test asserts the **parsed field**, and its `raw` assertion is there to +show why `raw` cannot be the witness rather than to check the writer. + +Fault injected: `build_formula`'s `Rule::format_clause` arm discarded instead of +recorded (`let _ = first_inner(inner);`). It fails with +`left: None right: Some(".4")` — and **every other test in `improvise-io` +passes**, 242 of them, including the whole `format_md`↔`parse_md` +characterization suite and the grammar-walking generator properties. That +measurement is the entry's point: no text-comparing test can see this field +go missing. + +Note what the injected fault does *not* do: it leaves the grammar alone, so the +line still parses and the file is still well-formed. A fault that also broke +the parse would fail the test for the wrong reason. + +### parse_md_reads_a_formula_with_no_format_as_none + +The absent case, so the field above is something the file carried rather than a +constant the walker invents. Without it, a walker hard-coding `Some(".4")` +would pass the test above. diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 6d298bd6..b13d4d92 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -406,6 +406,7 @@ mod tests { target_category: "Type".to_string(), expr: crate::formula::ast::Expr::Number(0.0), filter: None, + format: None, }); let layout = make_layout(&m); let reg = make_registry(); diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 297d6ce3..1b21cce0 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -17,7 +17,7 @@ use std::rc::Rc; use crate::command::cmd::CmdContext; use crate::command::keymap::{Keymap, KeymapSet}; -use crate::format::parse_number_format; +use crate::format::FormatPlan; use crate::import::wizard::ImportWizard; use crate::model::cell::{CellKey, CellValue, DataStore}; use crate::persistence; @@ -796,13 +796,8 @@ impl App { display_value: self.display_value_at_cursor(), visible_rows: (self.term_height as usize).saturating_sub(8), visible_cols: { - let (fmt_comma, fmt_decimals) = parse_number_format(&view.number_format); - let col_widths = compute_col_widths( - &self.model_state.workbook.model, - layout, - fmt_comma, - fmt_decimals, - ); + let model = &self.model_state.workbook.model; + let col_widths = compute_col_widths(model, layout, &FormatPlan::of(model, view)); let row_header_width = compute_row_header_width(layout); compute_visible_cols( &col_widths, diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index a35bfdaf..387ed955 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -453,7 +453,7 @@ mod tests { let mut seen = vec![connect.msg]; seen.extend(to(&frames, id)); - assert_eq!(cell_text(&folded_grid(&seen), 0, 0), "42"); + assert_eq!(cell_text(&folded_grid(&seen), 0, 0), "42.00"); } /// `doctrine(check_the_checker)`: inject the fault — never send the command — @@ -483,7 +483,7 @@ mod tests { let mut seen = vec![bootstrap.msg]; seen.extend(to(&frames, bystander)); - assert_eq!(cell_text(&folded_grid(&seen), 0, 0), "7"); + assert_eq!(cell_text(&folded_grid(&seen), 0, 0), "7.00"); } /// A view-only command reaches nobody else — the counterpart that keeps the diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index fe438bf0..ddf4c6ff 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -3,7 +3,7 @@ use std::collections::BTreeMap; -use improvise_core::format::{format_f64, format_value, parse_number_format}; +use improvise_core::format::FormatPlan; use improvise_core::model::Model; use improvise_core::model::category::MEASURE_CATEGORY; use improvise_core::model::cell::CellValue; @@ -141,12 +141,12 @@ pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache { let elsewhere = (name != app.view_name()).then(|| GridLayout::with_frozen_records(model, view, None)); let layout = elsewhere.as_ref().unwrap_or(&app.layout); - let (comma, decimals) = parse_number_format(&view.number_format); + let plan = FormatPlan::of(model, view); let rows = viewport.rows_in(layout.row_count()); let cols = viewport.cols_in(layout.col_count()); // ONE walk: the widths, the totals and this window's values all come off the // same evaluation — `context/code/command.md` (improvise-jmjz). - let body = grid_body(model, layout, comma, decimals, rows.clone(), cols.clone()); + let body = grid_body(model, layout, &plan, rows.clone(), cols.clone()); GridCache { row_frame: axis_frame(&layout.row_cats, &layout.row_items, view, rows.clone()), @@ -161,11 +161,11 @@ pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache { .map(|(r, values)| { cols.clone() .zip(values.iter()) - .map(|(c, value)| cell_display(app, layout, r, c, value, comma, decimals)) + .map(|(c, value)| cell_display(app, layout, r, c, value, &plan)) .collect() }) .collect(), - totals: column_totals(&body, cols, comma, decimals), + totals: column_totals(&body, cols), page_coords: layout.page_coords.clone(), // The whole grid, not the window: this is what a client clamps its own // cursor against (improvise-bpq2). Off the layout, not the clipped ranges. @@ -252,17 +252,12 @@ fn axis_frame( /// The totals strip: each visible column summed over **every** row, not just the /// viewport's — which is why it is projected. Empty means "no totals" (gotcha 33). -fn column_totals( - body: &GridBody, - cols: std::ops::Range, - comma: bool, - decimals: u8, -) -> Vec { +fn column_totals(body: &GridBody, cols: std::ops::Range) -> Vec { match body.totals.is_empty() { true => Vec::new(), false => cols - .filter_map(|c| body.totals.get(c)) - .map(|total| CellDisplay::number(format_f64(*total, comma, decimals))) + .filter_map(|c| Some((body.totals.get(c)?, body.col_formats.get(c)?))) + .map(|(total, fmt)| CellDisplay::number(fmt.number(*total))) .collect(), } } @@ -275,8 +270,7 @@ fn cell_display( row: usize, col: usize, value: &Option, - comma: bool, - decimals: u8, + plan: &FormatPlan, ) -> CellDisplay { if layout.is_records_mode() { // The value, not its spelling: this module used to infer numeric-ness @@ -290,16 +284,15 @@ fn cell_display( id: cell_id(app, layout, row, col), }; } + let key = layout.cell_key(row, col); CellDisplay { - text: format_value(value.as_ref(), comma, decimals), + text: plan.at(key.as_ref()).value(value.as_ref()), numeric: matches!(value, Some(CellValue::Number(_))), error: matches!(value, Some(CellValue::Error(_))), id: cell_id(app, layout, row, col), // The *unformatted* value the TUI seeds its edit buffer from. Projecting it // beats a client un-formatting `text` (gotcha 47). - edit_text: layout - .cell_key(row, col) - .map(|key| app.display_value_in(layout, &key)), + edit_text: key.map(|key| app.display_value_in(layout, &key)), } } @@ -1811,7 +1804,7 @@ mod tests { .collect(); assert_eq!(cells.len(), 1, "expected one changed cell, got {cells:?}"); assert_eq!(cells[0].0, (1, 1)); - assert_eq!(cells[0].1.text, "7"); + assert_eq!(cells[0].1.text, "7.00"); } /// Scrolling is `ViewOnly` — no other client hears it — but the client @@ -2733,7 +2726,8 @@ mod tests { let model = &app.model_state.workbook.model; assert_eq!(app.layout.row_count(), 3, "precondition: three rows"); - let one_row = grid_body(model, &app.layout, false, 0, 0..1, 0..1); + let plan = improvise_core::format::FormatPlan::uniform(""); + let one_row = grid_body(model, &app.layout, &plan, 0..1, 0..1); assert_eq!( one_row.window.len(), 1, @@ -2752,7 +2746,7 @@ mod tests { ); assert_eq!( one_row.col_widths, - compute_col_widths(model, &app.layout, false, 0), + compute_col_widths(model, &app.layout, &plan), "the keypress path and the projection must size a column the same" ); } @@ -4396,7 +4390,7 @@ mod cell_identity { assert_eq!( grid_of(&app, WHOLE).cell(row, 0).map(|c| c.text.clone()), - Some("42".to_string()) + Some("42.00".to_string()) ); } @@ -4432,7 +4426,7 @@ mod cell_identity { vec![3], "exactly (1, 1) may move: {before:?} {after:?}" ); - assert_eq!(after[3], "999"); + assert_eq!(after[3], "999.00"); } /// A surrogate that does not resolve applies *nothing*. The floor underneath the @@ -4510,6 +4504,39 @@ mod cell_identity { app } + /// improvise-e0ex: a formula's own format reaches the drawn grid — cells and + /// the totals strip — and stops there. `context/test-evidence/command.md`. + #[test] + fn a_formulas_format_reaches_its_own_column_and_no_other() { + let mut wb = raw_workbook(); + wb.model.add_formula( + improvise_formula::parse_formula("Margin = Revenue / Cost FORMAT .4", "_Measure") + .unwrap(), + ); + let app = App::new(wb, None); + let grid = grid_of(&app, WHOLE); + + let margin = column_named(&app, "Margin"); + let revenue = column_named(&app, "Revenue"); + assert_eq!( + grid.cell(0, margin).map(|c| c.text.clone()), + Some("0.5000".to_string()), + "the formula's own four decimals, not the view's two" + ); + assert_eq!( + grid.cell(0, revenue).map(|c| c.text.clone()), + Some("1.00".to_string()), + "a raw measure keeps the view's format" + ); + // The totals strip is formatted on its own path (`col_formats`), so it + // can disagree with the cells above it without any cell being wrong. + assert_eq!( + grid.totals[margin].text, "1.5000", + "three rows of 0.5, under the column's own format" + ); + assert_eq!(grid.totals[revenue].text, "3.00"); + } + fn column_named(app: &App, label: &str) -> usize { (0..app.layout.col_count()) .find(|&c| app.layout.col_label(c) == label) @@ -5040,7 +5067,7 @@ mod basis_check { .cells .iter() .flatten() - .any(|c| c.text == "99"), + .any(|c| c.text == "99.00"), "the accepted write never reached the document" ); } @@ -5077,7 +5104,7 @@ mod basis_check { assert_eq!(rejection(applied), Rejection::RecordMoved(target.record)); assert_eq!( value_at(&app, 1), - "7", + "7.00", "a rejected batch must leave the document exactly as it was" ); } @@ -5106,7 +5133,7 @@ mod basis_check { matches!(applied, Applied::Diff { .. }), "another row's edit invalidated this one: {applied:?}" ); - assert_eq!(value_at(&app, 1), "99"); + assert_eq!(value_at(&app, 1), "99.00"); } /// Class 2 by the other route: "gone" and "changed" must not collapse into one @@ -5146,7 +5173,11 @@ mod basis_check { let applied = sessions.apply(&mut app, id, &write(target.clone(), basis)); assert_eq!(rejection(applied), Rejection::RecordUnstated(target.record)); - assert_eq!(value_at(&app, 1), "1", "the omitted claim let a write land"); + assert_eq!( + value_at(&app, 1), + "1.00", + "the omitted claim let a write land" + ); } // ── tier 1: the document ──────────────────────────────────────────── @@ -5273,7 +5304,7 @@ mod basis_check { matches!(applied, Applied::Diff { .. }), "a basis composed only from the published stream was refused: {applied:?}" ); - assert_eq!(value_at(&app, 1), "99"); + assert_eq!(value_at(&app, 1), "99.00"); } /// The other side of publication: after somebody else writes, the stream carries @@ -5331,6 +5362,6 @@ mod basis_check { matches!(applied, Applied::Diff { .. }), "a re-composed batch was refused: {applied:?}" ); - assert_eq!(value_at(&app, 1), "99"); + assert_eq!(value_at(&app, 1), "99.00"); } } diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index c6ca72c0..20f8a5f8 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -5,8 +5,8 @@ use unicode_width::UnicodeWidthStr; use improvise_protocol::header_level_widths; -use crate::format::format_f64; -use crate::model::cell::CellValue; +use crate::format::{FormatPlan, NumberFormat}; +use crate::model::cell::{CellKey, CellValue}; use crate::model::{Aggregation, Model}; use crate::view::{AxisEntry, GridLayout}; @@ -23,6 +23,9 @@ pub use improvise_protocol::{MAX_ROW_HEADER_W, MIN_ROW_HEADER_W}; pub struct GridBody { /// One per column of the WHOLE grid, clamped, never windowed. pub col_widths: Vec, + /// The format each column's cells were drawn with, read off the top row so + /// the totals strip agrees with them — `context/code/command.md`. + pub col_formats: Vec, /// Column sums over every row. **Empty** in records mode or an empty grid, /// which is "this grid has no totals" rather than a strip of zeroes. pub totals: Vec, @@ -35,8 +38,7 @@ pub struct GridBody { pub fn grid_body( model: &Model, layout: &GridLayout, - fmt_comma: bool, - fmt_decimals: u8, + plan: &FormatPlan, rows: std::ops::Range, cols: std::ops::Range, ) -> GridBody { @@ -49,6 +51,7 @@ pub fn grid_body( false => Vec::new(), }; let mut window: Vec>> = Vec::with_capacity(rows.len()); + let mut col_formats: Vec = vec![plan.view_format(); n]; for ri in 0..layout.row_count() { let drawn = rows.contains(&ri); @@ -57,10 +60,14 @@ pub fn grid_body( false => Vec::new(), }; for ci in 0..n { - let value = cell_value(model, layout, records, ri, ci); + let (key, value) = cell_value(model, layout, records, ri, ci); + let fmt = plan.at(key.as_ref()); + if let Some(slot) = col_formats.get_mut(ci).filter(|_| ri == 0) { + *slot = fmt; + } let text = match records { true => value.as_ref().map(|v| v.to_string()).unwrap_or_default(), - false => crate::format::format_value(value.as_ref(), fmt_comma, fmt_decimals), + false => fmt.value(value.as_ref()), }; widen(&mut widths, ci, text.width()); if let Some(sum) = totals.get_mut(ci) { @@ -75,8 +82,8 @@ pub fn grid_body( } } for (ci, total) in totals.iter().enumerate() { - let width = format_f64(*total, fmt_comma, fmt_decimals).width(); - widen(&mut widths, ci, width); + let fmt = col_formats.get(ci).copied().unwrap_or(plan.view_format()); + widen(&mut widths, ci, fmt.number(*total).width()); } GridBody { @@ -84,6 +91,7 @@ pub fn grid_body( .into_iter() .map(|w| (w + 1).clamp(MIN_COL_WIDTH, MAX_COL_WIDTH)) .collect(), + col_formats, totals, window, } @@ -96,12 +104,16 @@ fn cell_value( records: bool, row: usize, col: usize, -) -> Option { +) -> (Option, Option) { match records { - true => layout.records_value(row, col), - false => layout - .cell_key(row, col) - .and_then(|key| model.evaluate_aggregated(&key, Aggregation::of(&layout.none_cats))), + true => (None, layout.records_value(row, col)), + false => { + let key = layout.cell_key(row, col); + let value = key + .as_ref() + .and_then(|k| model.evaluate_aggregated(k, Aggregation::of(&layout.none_cats))); + (key, value) + } } } @@ -131,13 +143,8 @@ fn widen(widths: &mut [u16], col: usize, width: usize) { /// Adaptive column widths alone, for the keypress path, which needs no cells. /// It is [`grid_body`] with an empty window so there is one width rule. -pub fn compute_col_widths( - model: &Model, - layout: &GridLayout, - fmt_comma: bool, - fmt_decimals: u8, -) -> Vec { - grid_body(model, layout, fmt_comma, fmt_decimals, 0..0, 0..0).col_widths +pub fn compute_col_widths(model: &Model, layout: &GridLayout, plan: &FormatPlan) -> Vec { + grid_body(model, layout, plan, 0..0, 0..0).col_widths } /// The total row-header width for a layout. The rule is the protocol's; this is the diff --git a/crates/improvise-core/src/format.rs b/crates/improvise-core/src/format.rs index fb148891..236afc89 100644 --- a/crates/improvise-core/src/format.rs +++ b/crates/improvise-core/src/format.rs @@ -1,4 +1,84 @@ -use crate::model::cell::CellValue; +use std::collections::HashMap; + +use crate::model::cell::{CellKey, CellValue}; +use crate::model::types::Model; +use crate::view::types::View; + +/// Comma grouping and decimal places — the pair every render path used to +/// thread as two scalars. `context/code/core.md`, "The number format". +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct NumberFormat { + pub comma: bool, + pub decimals: u8, +} + +impl NumberFormat { + pub fn parse(fmt: &str) -> Self { + let (comma, decimals) = parse_number_format(fmt); + Self { comma, decimals } + } + + pub fn value(&self, v: Option<&CellValue>) -> String { + format_value(v, self.comma, self.decimals) + } + + pub fn number(&self, n: f64) -> String { + format_f64(n, self.comma, self.decimals) + } +} + +/// Which format each cell is drawn with: the view's `format:`, unless one of the +/// cell's coordinates names a formula carrying its own (`context/code/core.md`). +#[derive(Clone, Debug)] +pub struct FormatPlan { + view: NumberFormat, + by_target: HashMap>, +} + +impl FormatPlan { + pub fn of(model: &Model, view: &View) -> Self { + let mut by_target: HashMap> = HashMap::new(); + for f in model.formulas() { + if let Some(fmt) = &f.format { + by_target + .entry(f.target_category.clone()) + .or_default() + .insert(f.target.clone(), NumberFormat::parse(fmt)); + } + } + Self { + view: NumberFormat::parse(&view.number_format), + by_target, + } + } + + /// One format for every cell — for the paths that have a format string and + /// no model to ask, and for tests that are not about the override. + pub fn uniform(fmt: &str) -> Self { + Self { + view: NumberFormat::parse(fmt), + by_target: HashMap::new(), + } + } + + /// The view's own answer: what a cell naming no formula target is drawn with. + pub fn view_format(&self) -> NumberFormat { + self.view + } + + /// Asked of the *key*, never of the axes: a cell aggregating over `_Measure` + /// carries no target coordinate and so keeps the view's format. + pub fn at(&self, key: Option<&CellKey>) -> NumberFormat { + match key.filter(|_| !self.by_target.is_empty()) { + None => self.view, + Some(key) => self + .by_target + .iter() + .find_map(|(cat, items)| items.get(key.get(cat)?).copied()) + .unwrap_or(self.view), + } + } +} /// Format a CellValue for display with number formatting options. pub fn format_value(v: Option<&CellValue>, comma: bool, decimals: u8) -> String { @@ -292,3 +372,63 @@ mod tests { assert_eq!(format_f64(f64::INFINITY, false, 2), "inf"); } } + +#[cfg(test)] +mod plan_tests { + use super::*; + use crate::formula::parse_formula; + use crate::model::cell::CellKey; + use crate::view::types::View; + + fn ratio() -> CellValue { + CellValue::Number(2.0 / 3.0) + } + + /// improvise-e0ex — see `context/test-evidence/core.md`. + #[test] + fn a_ratio_keeps_its_decimals_under_a_fresh_views_format() { + let plan = FormatPlan::uniform(&View::new("Default").number_format); + assert_eq!(plan.view_format().value(Some(&ratio())), "0.67"); + } + + fn model_with(raw: &str) -> Model { + let mut m = Model::new("m"); + m.add_formula(parse_formula(raw, "_Measure").expect("parses")); + m + } + + fn measure(item: &str) -> CellKey { + CellKey::new(vec![("_Measure".to_string(), item.to_string())]) + } + + /// improvise-e0ex — see `context/test-evidence/core.md`. + #[test] + fn a_formulas_own_format_wins_for_its_target_and_nowhere_else() { + let model = model_with("Margin = Profit / Revenue FORMAT .4"); + let view = View::new("Default"); + let plan = FormatPlan::of(&model, &view); + + assert_eq!( + plan.at(Some(&measure("Margin"))).value(Some(&ratio())), + "0.6667" + ); + // The raw measure beside it keeps the view's format, and so does a cell + // naming no measure at all — the two halves of "nowhere else". + assert_eq!( + plan.at(Some(&measure("Revenue"))).value(Some(&ratio())), + "0.67" + ); + assert_eq!(plan.at(None).value(Some(&ratio())), "0.67"); + } + + /// improvise-e0ex — see `context/test-evidence/core.md`. + #[test] + fn a_formula_naming_no_format_leaves_its_target_on_the_views() { + let model = model_with("Margin = Profit / Revenue"); + let plan = FormatPlan::of(&model, &View::new("Default")); + assert_eq!( + plan.at(Some(&measure("Margin"))).value(Some(&ratio())), + "0.67" + ); + } +} diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 7be0f959..00e0843d 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -547,15 +547,17 @@ impl GridLayout { model: &Model, row: usize, col: usize, - fmt_comma: bool, - fmt_decimals: u8, + plan: &crate::format::FormatPlan, ) -> String { if self.is_records_mode() { self.records_display(row, col).unwrap_or_default() } else { self.cell_key(row, col) - .and_then(|key| model.evaluate_aggregated(&key, Aggregation::of(&self.none_cats))) - .map(|v| crate::format::format_value(Some(&v), fmt_comma, fmt_decimals)) + .and_then(|key| { + let value = + model.evaluate_aggregated(&key, Aggregation::of(&self.none_cats))?; + Some(plan.at(Some(&key)).value(Some(&value))) + }) .unwrap_or_default() } } @@ -809,7 +811,10 @@ mod tests { !layout.none_cats.is_empty(), "a default view must have None-axis categories for this to be the bug" ); - assert_eq!(layout.display_text(&wb.model, 0, 0, false, 0), "pending"); + assert_eq!( + layout.display_text(&wb.model, 0, 0, &crate::format::FormatPlan::uniform("")), + "pending" + ); } #[test] diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index fdc79d55..550ad4a1 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -53,7 +53,9 @@ impl View { page_selections: HashMap::new(), hidden_items: HashMap::new(), collapsed_groups: HashMap::new(), - number_format: ",.0".to_string(), + // Decimals by default: a ratio is the case a format cannot be + // guessed for, and `.0` draws 2/3 as `1` (improvise-e0ex). + number_format: ",.2".to_string(), prune_empty: false, } } diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 90482721..1fe4bad1 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -57,6 +57,10 @@ pub struct Formula { pub expr: Expr, /// Optional WHERE filter pub filter: Option, + /// How this formula's target is drawn, overriding the view's `format:` for + /// that one measure. `None` means the view answers — `context/code/core.md`. + #[serde(default)] + pub format: Option, } impl Formula { @@ -73,6 +77,14 @@ impl Formula { target_category: target_category.into(), expr, filter, + format: None, } } + + /// The same formula drawn with `format`. Separate from [`Formula::new`] so + /// the five-argument constructor every caller already spells stays put. + pub fn with_format(mut self, format: Option) -> Self { + self.format = format; + self + } } diff --git a/crates/improvise-formula/src/formula.pest b/crates/improvise-formula/src/formula.pest index 38d11d81..d3c38a47 100644 --- a/crates/improvise-formula/src/formula.pest +++ b/crates/improvise-formula/src/formula.pest @@ -13,7 +13,7 @@ WHITESPACE = _{ " " | "\t" } // ---- top-level ---------------------------------------------------------- -formula = { SOI ~ target ~ "=" ~ expr ~ where_clause? ~ EOI } +formula = { SOI ~ target ~ "=" ~ expr ~ where_clause? ~ format_clause? ~ EOI } // The target is an ordinary identifier: parser.rs unquotes it exactly the // way it unquotes a `ref_expr`, so `|Total Revenue| = ...` and a reference @@ -24,6 +24,11 @@ target = { identifier } // `WHEREAS x = y` reads as a WHERE on a category `AS`. where_clause = ${ ^"WHERE" ~ sp+ ~ identifier ~ sp* ~ "=" ~ sp* ~ filter_value } +// How this formula's TARGET is drawn, overriding the view's `format:` for that +// one measure. `sp+` for the same reason `WHERE` has it — `context/code/core.md`. +format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec } +format_spec = @{ (ASCII_ALPHANUMERIC | "," | "." | "%")+ } + // ---- expressions -------------------------------------------------------- // Used by parse_expr() — forces a standalone expression to consume the diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index d4256ad1..38498fbf 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -42,6 +42,7 @@ fn build_formula(pair: Pair, raw: &str, target_category: &str) -> Formula let mut target = None; let mut expr = None; let mut filter = None; + let mut format = None; for inner in pair.into_inner() { match inner.as_rule() { // Unquoted exactly like `Expr::Ref`: the same spelling must name @@ -49,6 +50,10 @@ fn build_formula(pair: Pair, raw: &str, target_category: &str) -> Formula Rule::target => target = Some(identifier_to_string(first_inner(inner))), Rule::expr => expr = Some(build_expr(inner)), Rule::where_clause => filter = Some(build_filter(inner)), + // The spec, not the clause: `FORMAT ` is syntax and not the picture. + Rule::format_clause => { + format = Some(first_inner(inner).as_str().to_string()); + } // Ignored as an ARM, so "ignored" is a decision and not a silence. Rule::EOI => {} Rule::WHITESPACE @@ -77,6 +82,7 @@ fn build_formula(pair: Pair, raw: &str, target_category: &str) -> Formula | Rule::pipe_quoted | Rule::bare_ident | Rule::filter_value + | Rule::format_spec | Rule::string | Rule::number => { unreachable!("{GRAMMAR_INVARIANT}: formula part {:?}", inner.as_rule()) @@ -90,6 +96,7 @@ fn build_formula(pair: Pair, raw: &str, target_category: &str) -> Formula expr.expect(GRAMMAR_INVARIANT), filter, ) + .with_format(format) } fn build_expr(pair: Pair) -> Expr { @@ -166,6 +173,8 @@ fn build_unary(pair: Pair) -> Expr { | Rule::pipe_quoted | Rule::bare_ident | Rule::filter_value + | Rule::format_clause + | Rule::format_spec | Rule::string | Rule::number => { unreachable!( @@ -214,6 +223,8 @@ fn build_primary(pair: Pair) -> Expr { | Rule::pipe_quoted | Rule::bare_ident | Rule::filter_value + | Rule::format_clause + | Rule::format_spec | Rule::string => { unreachable!( "{GRAMMAR_INVARIANT}: primary alternative {:?}", @@ -324,6 +335,8 @@ fn filter_value_to_string(pair: Pair) -> String { | Rule::ref_expr | Rule::identifier | Rule::filter_value + | Rule::format_clause + | Rule::format_spec | Rule::number => { unreachable!( "{GRAMMAR_INVARIANT}: filter_value alternative {:?}", diff --git a/crates/improvise-formula/src/quote.rs b/crates/improvise-formula/src/quote.rs index 81b49eda..cdc05bc0 100644 --- a/crates/improvise-formula/src/quote.rs +++ b/crates/improvise-formula/src/quote.rs @@ -3,7 +3,9 @@ /// Words the formula grammar gives a meaning of their own. A name spelled like /// one is pipe-quoted even though it is otherwise a legal bare identifier. -const KEYWORDS: [&str; 8] = ["WHERE", "SUM", "AVG", "MIN", "MAX", "COUNT", "IF", "IS"]; +const KEYWORDS: [&str; 9] = [ + "WHERE", "SUM", "AVG", "MIN", "MAX", "COUNT", "IF", "IS", "FORMAT", +]; /// Bare-writable: matches `[A-Za-z_][A-Za-z0-9_-]*` and is not a [`KEYWORDS`] /// entry. The keyword clause is canonical — `context/code/formula-and-io.md`. diff --git a/crates/improvise-gp/src/dataset.rs b/crates/improvise-gp/src/dataset.rs index 1d3842ba..d7144cbc 100644 --- a/crates/improvise-gp/src/dataset.rs +++ b/crates/improvise-gp/src/dataset.rs @@ -453,11 +453,10 @@ mod tests { /// `f64`, so a value the grid shows as "5" must reach the table as 5.25. #[test] fn a_cell_reaches_the_table_unrounded() { - let (mut model, view) = fixture(); - assert_eq!( - view.number_format, ",.0", - "the fixture view rounds to integers" - ); + let (mut model, mut view) = fixture(); + // Pinned, not inherited: the default shows two decimals since + // improvise-e0ex, and 5.25 under it is not rounded at all. + view.number_format = ",.0".to_string(); model.set_cell( record_key("North", "Web", "Q1", "n1", "Units"), CellValue::Number(5.25), diff --git a/crates/improvise-io/src/persistence/export.rs b/crates/improvise-io/src/persistence/export.rs index 83d00f1a..993de670 100644 --- a/crates/improvise-io/src/persistence/export.rs +++ b/crates/improvise-io/src/persistence/export.rs @@ -34,7 +34,7 @@ fn render_csv(workbook: &Workbook, view_name: &str) -> Result> { // 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); + let plan = crate::format::FormatPlan::of(model, view); let mut wtr = csv::WriterBuilder::new() .flexible(true) @@ -66,10 +66,7 @@ fn render_csv(workbook: &Workbook, view_name: &str) -> Result> { if !row_label.is_empty() { record.push(row_label); } - record.extend( - (0..layout.col_count()) - .map(|ci| layout.display_text(model, ri, ci, fmt_comma, fmt_decimals)), - ); + record.extend((0..layout.col_count()).map(|ci| layout.display_text(model, ri, ci, &plan))); wtr.write_record(&record)?; } diff --git a/crates/improvise-io/src/persistence/roundtrip_tests.rs b/crates/improvise-io/src/persistence/roundtrip_tests.rs index 654af3f9..f0a05ad6 100644 --- a/crates/improvise-io/src/persistence/roundtrip_tests.rs +++ b/crates/improvise-io/src/persistence/roundtrip_tests.rs @@ -1121,3 +1121,40 @@ fn a_cell_with_no_coordinates_round_trips() { ); assert_eq!(format_md(&back), text); } + +// ── parse_md: a formula's own number format (improvise-e0ex) ───────── + +/// The PARSED format, never the re-emitted line — `raw` cannot be the +/// witness (gotcha 34). Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn parse_md_round_trips_the_format_a_formula_carries() { + let mut m = two_cat_model(); + m.model + .add_formula(parse_formula("Margin = Revenue / Cost FORMAT .4", "_Measure").unwrap()); + + let text = format_md(&m); + let loaded = parse_md(&text).unwrap(); + + let f = &loaded.model.formulas()[0]; + assert_eq!( + f.format.as_deref(), + Some(".4"), + "the format must survive as a parsed field, not only inside `raw`" + ); + // The two halves this test exists to keep apart: `raw` is byte-identical + // whether or not the walker kept the field, so it cannot be the witness. + assert_eq!(f.raw, "Margin = Revenue / Cost FORMAT .4"); + assert_eq!(f.target, "Margin"); +} + +/// A formula naming no format reads back as `None`, so the field above is a +/// decision the file carried. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn parse_md_reads_a_formula_with_no_format_as_none() { + let mut m = two_cat_model(); + m.model + .add_formula(parse_formula("Margin = Revenue / Cost", "_Measure").unwrap()); + + let loaded = parse_md(&format_md(&m)).unwrap(); + assert_eq!(loaded.model.formulas()[0].format, None); +} diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index d4651c9c..d6bbd163 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -120,7 +120,7 @@ fn a_browser_client_seeds_types_a_number_and_sees_it_come_back() { assert_eq!(model.col_levels[0], vec!["Revenue", "Cost"]); assert_eq!( model.rows[0].cells()[0].text, - "1,200", + "1,200.00", "the value arrives *formatted* — the separator is the server's view \ format, and a client that re-derived the text would not have it" ); @@ -168,7 +168,7 @@ fn a_browser_client_seeds_types_a_number_and_sees_it_come_back() { ); assert_eq!( client.model().rows[at.visual_row].cells()[at.col].text, - "7,500", + "7,500.00", "the page redraws from the server's formatting, never from the \ \"7500\" the user typed" ); diff --git a/crates/improvise-worker-server/src/worker.rs b/crates/improvise-worker-server/src/worker.rs index a0263f00..da8edd75 100644 --- a/crates/improvise-worker-server/src/worker.rs +++ b/crates/improvise-worker-server/src/worker.rs @@ -358,7 +358,7 @@ mod tests { fn a_wire_edit_reaches_the_document_and_comes_back_as_a_projection() { let (mut worker, _) = worker_over_storage(); let mut grid = connect(&mut worker); - assert_eq!(grid.cells[0][0].text, "10"); + assert_eq!(grid.cells[0][0].text, "10.00"); let cell = addressed(&grid, 0, 0); let cmd = reduce::caught_up( @@ -371,7 +371,7 @@ mod tests { let out = worker.on_frame(ClientFrame::command(cmd)); let grid = folded(&mut grid, &out); assert_eq!( - grid.cells[0][0].text, "42", + grid.cells[0][0].text, "42.00", "the edit did not come back through the projection stream" ); } diff --git a/crates/improvise-worker-server/tests/post_message.rs b/crates/improvise-worker-server/tests/post_message.rs index 8c53a637..88763541 100644 --- a/crates/improvise-worker-server/tests/post_message.rs +++ b/crates/improvise-worker-server/tests/post_message.rs @@ -163,7 +163,7 @@ fn the_worker_loads_projects_edits_and_persists_over_post_message() { main.grid.row_frame.leaf_labels(), vec!["North", "South", "East"] ); - assert_eq!(main.text_at(0, 0), "10"); + assert_eq!(main.text_at(0, 0), "10.00"); let cell = main.cell(2, 0); main.post(&main.caught_up(ModelCommand::SetCell { @@ -172,7 +172,7 @@ fn the_worker_loads_projects_edits_and_persists_over_post_message() { })); assert_eq!( main.text_at(2, 0), - "31", + "31.00", "the edit must come back through the projection stream, not be assumed" ); @@ -227,7 +227,7 @@ fn a_reload_over_the_same_origin_reopens_what_was_saved() { reloaded.post(&ClientFrame::Viewport { rows: 10, cols: 4 }); assert_eq!( reloaded.text_at(0, 0), - "77", + "77.00", "the reload re-seeded over the user's saved work" ); } @@ -258,7 +258,7 @@ fn a_batch_composed_against_a_moved_document_is_refused() { ); assert_eq!( main.text_at(0, 0), - "2", + "2.00", "a refused batch changed the document" ); } @@ -336,7 +336,7 @@ fn the_shipped_demo_seeds_a_worker_whose_raw_cells_are_settable() { main.grid.col_frame.leaf_labels(), vec!["Revenue", "Cost", "Profit"] ); - assert_eq!(main.text_at(0, 2), "12,200", "Profit is live on arrival"); + assert_eq!(main.text_at(0, 2), "12,200.00", "Profit is live on arrival"); // `cell` panics on anything but a stored cell, which is exactly the failure a // visitor meets as a cell that does not answer a click (ADR 002). @@ -362,8 +362,8 @@ fn the_shipped_demo_seeds_a_worker_whose_raw_cells_are_settable() { cell: revenue, value: CellValue::Number(20000.0), })); - assert_eq!(main.text_at(0, 0), "20,000"); - assert_eq!(main.text_at(0, 2), "13,800", "20,000 - 6,200"); + assert_eq!(main.text_at(0, 0), "20,000.00"); + assert_eq!(main.text_at(0, 2), "13,800.00", "20,000 - 6,200"); } /// improvise-z754: `on_frame` built the client's frames and persisted AFTER, so diff --git a/crates/improvise-ws-server/tests/socket.rs b/crates/improvise-ws-server/tests/socket.rs index a3339d03..53eb59cc 100644 --- a/crates/improvise-ws-server/tests/socket.rs +++ b/crates/improvise-ws-server/tests/socket.rs @@ -133,7 +133,7 @@ fn a_client_on_a_socket_is_seeded_and_gets_its_edit_back() { waited += 1; } assert_eq!( - grid.cells[0][0].text, "12", + grid.cells[0][0].text, "12.00", "the edit never came back down the socket" ); diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index b679cfac..1c4bfe17 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -159,7 +159,7 @@ test.describe("every field the render model projects reaches the screen", () => // The totals strip. Q1 Revenue is 18400+24600+9200+11500 — a sum over // EVERY row, which is why it is projected and not added up here. await expect(page.locator("#grid tfoot th")).toHaveText("Total"); - await expect(page.locator("#grid tfoot td").first()).toHaveText("63,700"); + await expect(page.locator("#grid tfoot td").first()).toHaveText("63,700.00"); // A total is not a cell anyone may type into. await expect(page.locator("#grid tfoot td.editable")).toHaveCount(0); @@ -170,7 +170,7 @@ test.describe("every field the render model projects reaches the screen", () => await expect(page.locator("#pages .page-axis")).toHaveText("Quarter = Q2 (2/4)", { timeout: 10_000, }); - await expect(page.locator("#grid tfoot td").first()).toHaveText("66,400"); + await expect(page.locator("#grid tfoot td").first()).toHaveText("66,400.00"); }); }); @@ -280,7 +280,7 @@ test.describe("a pointer event reaches the client and its answer is painted", () // The third edit of the same repair: `close` restores the DRAWN text, not // the seed. `repaint` early-returns while `editing` is set, so a `18400` // put back here stays on screen (improvise-0x9u). - await expect(cells.nth(0)).toHaveText("18,400"); + await expect(cells.nth(0)).toHaveText("18,400.00"); await page.keyboard.press("Escape"); await expect(cells.nth(4)).toHaveClass(/selected/); diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index ef154f63..9648ea92 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -273,8 +273,8 @@ test.describe("the standalone deployment", () => { await input.fill("20000"); await input.press("Enter"); - await expect(cell).toHaveText("20,000", { timeout: 10_000 }); - expect(before).not.toBe("20,000"); + await expect(cell).toHaveText("20,000.00", { timeout: 10_000 }); + expect(before).not.toBe("20,000.00"); // The editor is gone and did not come back: the half of the re-entry guard // that IS observable from the DOM. await expect(page.locator("#grid input")).toHaveCount(0); diff --git a/src/ui/grid.rs b/src/ui/grid.rs index fb844f3c..255f3863 100644 --- a/src/ui/grid.rs +++ b/src/ui/grid.rs @@ -883,6 +883,9 @@ mod tests { m.add_item_in_group("Month", "Jan", "Q1").unwrap(); m.add_item_in_group("Month", "Apr", "Q2").unwrap(); m.add_item("Payer", "Alice").unwrap(); + // Pinned so the byte-exact frames below characterize the LAYOUT and not + // the view default, which grew two decimals with improvise-e0ex. + m.active_view_mut().number_format = ",.0".to_string(); for (r, v) in [("East", 100.0), ("West", 50.0)] { for mo in ["Jan", "Apr"] { m.model.set_cell( diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index 7fc16442..74d9b836 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -384,8 +384,7 @@ fn wide_cell_clamps_without_overflow() { let widths = improvise::ui::viewport::compute_col_widths( &app.model_state.workbook.model, &app.layout, - false, - 0, + &improvise::format::FormatPlan::uniform(""), ); assert_eq!( widths, diff --git a/tests/web_demo_model.rs b/tests/web_demo_model.rs index 3a5ffaa7..258974bd 100644 --- a/tests/web_demo_model.rs +++ b/tests/web_demo_model.rs @@ -76,5 +76,8 @@ fn the_web_demo_shows_a_formula_beside_its_inputs() { vec!["Espresso", "Latte", "Cold Brew", "Pastry"] ); let profit = grid.cell(0, 2).expect("the Profit cell"); - assert_eq!(profit.text, "12,200", "18,400 - 6,200, recomputed on load"); + assert_eq!( + profit.text, "12,200.00", + "18,400 - 6,200, recomputed on load, under the decimal default" + ); } diff --git a/web/demo.improv b/web/demo.improv index b1e14a28..9b865a69 100644 --- a/web/demo.improv +++ b/web/demo.improv @@ -8,7 +8,7 @@ _Measure: column Quarter: page, Q1 _Index: none _Dim: none -format: ,.0 +format: ,.2 ## View: Season Quarter: row @@ -16,7 +16,7 @@ _Measure: column Product: page, Latte _Index: none _Dim: none -format: ,.0 +format: ,.2 ## Formulas - Profit = Revenue - Cost From 2329dc6204c6b843b4989e961da12d9fd24390e8 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 23:45:07 -0700 Subject: [PATCH 2/3] fix: resolve a cell's format through the formula that owns it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from ChatGPT Codex's review of PR #125. Both are real, and both are the same mistake in two places: answering "whose format is this" from something other than the rule that answers "whose value is this". **1. `FormatPlan` indexed formats in a `HashMap` keyed by target.** A target category need not be `_Measure` — `[CustomCat]` is in the format's own documentation — so a key can carry coordinates two formulas claim, and iterating the map answered from whichever category hashed first: the displayed format was nondeterministic per process while the value was not. That is gotcha 18's shape. Worse, only *formatted* formulas were indexed, so a later formula's `FORMAT` reached a cell whose value an earlier unformatted formula produces. `FormatPlan` now holds every formula's `(target_category, target, Option)` in the model's own order and takes the first coordinate match — `Model::formula_for`'s rule and order exactly. A match naming no format falls through to the view; it is a match, not a miss. The list is dropped entirely when no formula names a format, so the common document still pays one `is_empty` per cell. **2. A column total took its format from row 0.** `context/code/core.md` argued that was safe because "where `_Measure` is on the row axis a column is not a measure at all". Wrong: a formula's target category is any category, so a formula can own a row-axis item — the total then inherited a row-local override for a row it had summed away, and re-ordering the rows changed its spelling. New `GridLayout::column_key(col)` gives the coordinates that survive the row aggregation (page/filter constraints plus the column's tuple), and `col_formats` is resolved from that, once per column, outside the row loop. Three regression tests, each with the rejected version injected as the fault: - `the_formula_that_owns_a_cell_names_its_format_even_when_it_names_none` puts the same two formulas in both orders, so preferring the formatted one fails one half and ignoring formats fails the other. - `the_owner_is_the_one_model_order_names_and_not_the_one_a_hash_does` rebuilds the plan five times, since each `HashMap` gets its own `RandomState`. - `a_column_total_does_not_inherit_a_row_local_format` builds one grid with the owned row first and one with it last and requires the strips to agree. Every raw cell holds the same value per measure, so the totals are numerically identical and only the format can differ. `context/code/core.md`'s wrong paragraph is replaced rather than amended, and says what it got wrong. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/core.md | 41 +++++-- context/test-evidence/command.md | 24 ++++ context/test-evidence/core.md | 36 ++++++ crates/improvise-command/src/ui/project.rs | 47 ++++++++ crates/improvise-command/src/ui/viewport.rs | 13 +- crates/improvise-core/src/format.rs | 125 ++++++++++++++++---- crates/improvise-core/src/view/layout.rs | 11 ++ 7 files changed, 263 insertions(+), 34 deletions(-) diff --git a/context/code/core.md b/context/code/core.md index 37e406fe..ec0b1f4c 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -870,15 +870,40 @@ which falls out of the key rather than needing a rule. Deriving it from "is with itself while disagreeing with the cells. **The empty plan short-circuits**, so a document with no formula format pays one -`is_empty` per cell and nothing else; the lookup is otherwise one hash get per -`(category, item)` pair in the plan, of which there is normally one. +`is_empty` per cell and nothing else; the lookup is otherwise a scan of the +formula list, which is short and must stay ordered for the reason below. -**A column total takes the format of its own column's first row.** `GridBody` -carries `col_formats` beside `col_widths` so the totals strip and the cells -above it cannot disagree. Where `_Measure` is on the *row* axis a column is not -a measure at all, so no formula format applies to any cell in it and the first -row's answer is the view's — the rule only has to be decided for the case it -cannot arise in. +**The owner is the formula `Model::formula_for` names, and nothing else.** +`FormatPlan` holds every formula's `(target_category, target, Option)` +in the model's own order and takes the **first** coordinate match, which is the +same rule and the same order evaluation uses to decide whose value the cell is. +Two things that shape enforces, both of which a `HashMap` keyed by target got +wrong and review caught (Codex, PR #125): + +- **A key may be claimed by two formulas**, because a target category need not + be `_Measure` — `- Tax = Revenue * 0.1 [CustomCat]` is in the format's own + documentation. Iterating a `HashMap` answered from whichever category hashed + first, so the *displayed* format was nondeterministic per process while the + *value* was not. That is gotcha 18 exactly: an index whose values are hashed + is for lookup, never for choosing between candidates. +- **An owner that names no format still owns the cell.** Indexing only the + formatted formulas let a later formula's `FORMAT` reach a cell whose value it + does not produce. So the list holds *every* formula and a match with + `format: None` falls through to the view — it is a match, not a miss. + +The list is dropped entirely when no formula names a format, so the common +document pays one `is_empty` per cell rather than a scan. + +**A column total is resolved from the column, never from a row.** +`GridLayout::column_key(col)` is the page/filter constraints plus the column's +own tuple — the coordinates that survive the row aggregation — and `GridBody` +carries the resulting `col_formats` so the strip and the cells cannot disagree. +Reading it off row 0 was the first version and is wrong twice over, also found +by review: a formula may target a **row-axis** item (its target category is any +category), so the total inherited a row-local override for a row it had summed +away, **and** re-ordering the rows changed the total's spelling. The guard is +`a_column_total_does_not_inherit_a_row_local_format`, which builds the same grid +with the owned row first and last and requires the two strips to agree. ### The grammar: `FORMAT ` in the formula, not in the `.improv` line diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index ec2d4875..01925789 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4244,3 +4244,27 @@ Two faults injected: The second is the one that earns the test its length: without it, the totals strip could round every formula column to the view's format and no assertion in the workspace would move. + +### a_column_total_does_not_inherit_a_row_local_format + +Found by review (ChatGPT Codex, PR #125). The first version of `grid_body` read +each column's total format off **row 0**, and `context/code/core.md` argued that +was safe because "where `_Measure` is on the row axis a column is not a measure +at all". That argument is wrong: a formula's target category is *any* category, +so a formula can own a **row-axis item**, and then row 0's format is a row-local +override applied to a total that has summed that row away — and re-ordering the +rows changes the total's spelling. + +The fixture builds the same grid twice with `12.5 FORMAT .4` owning the **first** +row in one and the **last** row in the other, and requires the two totals strips +to agree. Every raw cell in `raw_workbook` holds the same value per measure, so +the two grids' totals are numerically identical and the *only* thing that can +differ is the format — the assertion has nowhere to hide. + +It asserts the owned cell first (`12.5000`), so a plan that applies no format at +all fails on the fixture rather than passing the comparison vacuously. + +Fault injected: `plan.at(layout.cell_key(0, ci).as_ref())` in place of +`layout.column_key(ci)` — the rejected version, restored. Fails with +`left: ["14.5000", "16.5000"] right: ["14.50", "16.50"]`, which is the bug in +one line: row order decided the spelling. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 23416970..4788a5e6 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1107,3 +1107,39 @@ The fallback, asserted where it is easiest to lose: a formula target whose formula carries no format must be indistinguishable from a raw measure. It keeps `FormatPlan::of` from indexing every target with some default and then "overriding" it back, which would be a second copy of the view's answer. + +### the_formula_that_owns_a_cell_names_its_format_even_when_it_names_none + +Found by review (ChatGPT Codex, PR #125), not by the tests above, which is the +entry's point: they used one formula and so could not see an ownership question +at all. + +A target category need not be `_Measure` — `- Tax = Revenue * 0.1 [CustomCat]` +is in the format's own documentation — so a key may carry coordinates two +formulas claim. The first `FormatPlan` indexed **only the formatted** formulas, +so a later formula's `FORMAT` reached a cell whose value an earlier, unformatted +formula produces: the number and its spelling came from different formulas. + +The fixture puts the same two formulas in both orders and asserts the owner's +answer each time, so a plan that prefers the formatted one fails the first half +and a plan that ignores formats entirely fails the second. + +Fault injected: `.filter(|t| t.format.is_some())` before the `find` — precisely +the behaviour of the version review rejected. Fails with `left: "0.6667" +right: "0.67"`. + +### the_owner_is_the_one_model_order_names_and_not_the_one_a_hash_does + +The other half of the same finding, and the one a single run cannot see. +`by_target` was a `HashMap>` and `at` iterated it, so +with two claiming categories the answer came from whichever hashed first — +**a different answer per process**, gotcha 18's shape exactly (an index whose +values are hashed is for lookup, never for choosing between candidates). + +It rebuilds the plan five times rather than asserting once, because each +`HashMap` gets its own `RandomState` and a single construction can be right by +luck. It is the same shape as `analyze_records`' five-iteration guard. + +This one cannot be made to fail by re-introducing a `HashMap` *reliably*, which +is why the test that carries the load is its sibling above; kept because the +repetition is what documents that hash order was the defect. diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index ddf4c6ff..c853db9a 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -4537,6 +4537,53 @@ mod cell_identity { assert_eq!(grid.totals[revenue].text, "3.00"); } + /// A formula owning one ROW, so its format is row-local and the totals have + /// aggregated that row away. `context/test-evidence/command.md`. + fn row_targeted_app(region: &str) -> App { + let mut wb = raw_workbook(); + wb.model.add_formula( + improvise_formula::parse_formula(&format!("{region} = 12.5 FORMAT .4"), "Region") + .unwrap(), + ); + App::new(wb, None) + } + + /// improvise-e0ex, found by review: reading a total's format off row 0 made + /// it depend on row ORDER. `context/test-evidence/command.md`. + #[test] + fn a_column_total_does_not_inherit_a_row_local_format() { + let first = row_targeted_app("North"); + let last = row_targeted_app("East"); + let (fg, lg) = (grid_of(&first, WHOLE), grid_of(&last, WHOLE)); + + // The row the formula owns really is drawn with the formula's format — + // without this the test would pass against a plan that does nothing. + let owned = fg.cell(0, 0).map(|c| c.text.clone()); + assert_eq!( + owned, + Some("12.5000".to_string()), + "fixture: North is owned" + ); + + // North is row 0 and East is the last row, so a total read off row 0 + // answers differently in the two grids. It must not. + let totals = |g: &improvise_protocol::render::GridCache| -> Vec { + g.totals.iter().map(|t| t.text.clone()).collect() + }; + assert_eq!( + totals(&fg), + totals(&lg), + "which row a formula owns changed the totals' spelling" + ); + for t in totals(&fg) { + assert_eq!( + t.split('.').nth(1).map(str::len), + Some(2), + "a total must keep the VIEW's two decimals, got {t:?}" + ); + } + } + fn column_named(app: &App, label: &str) -> usize { (0..app.layout.col_count()) .find(|&c| app.layout.col_label(c) == label) diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index 20f8a5f8..50bb2522 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -23,8 +23,8 @@ pub use improvise_protocol::{MAX_ROW_HEADER_W, MIN_ROW_HEADER_W}; pub struct GridBody { /// One per column of the WHOLE grid, clamped, never windowed. pub col_widths: Vec, - /// The format each column's cells were drawn with, read off the top row so - /// the totals strip agrees with them — `context/code/command.md`. + /// The format each column's TOTAL is drawn with, resolved from the + /// coordinates that survive the row aggregation — `context/code/core.md`. pub col_formats: Vec, /// Column sums over every row. **Empty** in records mode or an empty grid, /// which is "this grid has no totals" rather than a strip of zeroes. @@ -51,7 +51,11 @@ pub fn grid_body( false => Vec::new(), }; let mut window: Vec>> = Vec::with_capacity(rows.len()); - let mut col_formats: Vec = vec![plan.view_format(); n]; + // Off the COLUMN, not off row 0: a formula targeting a row-axis item is a + // row-local override, and the total has aggregated that row away. + let col_formats: Vec = (0..n) + .map(|ci| plan.at(layout.column_key(ci).as_ref())) + .collect(); for ri in 0..layout.row_count() { let drawn = rows.contains(&ri); @@ -62,9 +66,6 @@ pub fn grid_body( for ci in 0..n { let (key, value) = cell_value(model, layout, records, ri, ci); let fmt = plan.at(key.as_ref()); - if let Some(slot) = col_formats.get_mut(ci).filter(|_| ri == 0) { - *slot = fmt; - } let text = match records { true => value.as_ref().map(|v| v.to_string()).unwrap_or_default(), false => fmt.value(value.as_ref()), diff --git a/crates/improvise-core/src/format.rs b/crates/improvise-core/src/format.rs index 236afc89..07c9b72f 100644 --- a/crates/improvise-core/src/format.rs +++ b/crates/improvise-core/src/format.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use crate::model::cell::{CellKey, CellValue}; use crate::model::types::Model; use crate::view::types::View; @@ -27,28 +25,44 @@ impl NumberFormat { } } -/// Which format each cell is drawn with: the view's `format:`, unless one of the -/// cell's coordinates names a formula carrying its own (`context/code/core.md`). +/// One formula's claim on a cell: the coordinate it owns, and the format it +/// asks for — `None` where it names none. `context/code/core.md`. +#[derive(Clone, Debug)] +struct TargetFormat { + category: String, + target: String, + format: Option, +} + +/// Which format each cell is drawn with: the view's `format:`, unless the +/// formula that OWNS the cell names its own (`context/code/core.md`). #[derive(Clone, Debug)] pub struct FormatPlan { view: NumberFormat, - by_target: HashMap>, + /// Every formula, in the model's own order, or **empty** where none names a + /// format — the ownership rule is `Model::formula_for`'s, not a hash's. + targets: Vec, } impl FormatPlan { pub fn of(model: &Model, view: &View) -> Self { - let mut by_target: HashMap> = HashMap::new(); - for f in model.formulas() { - if let Some(fmt) = &f.format { - by_target - .entry(f.target_category.clone()) - .or_default() - .insert(f.target.clone(), NumberFormat::parse(fmt)); - } - } + let targets: Vec = model + .formulas() + .iter() + .map(|f| TargetFormat { + category: f.target_category.clone(), + target: f.target.clone(), + format: f.format.as_deref().map(NumberFormat::parse), + }) + .collect(); Self { view: NumberFormat::parse(&view.number_format), - by_target, + // Where no formula names a format every answer is the view's, so + // the list is dropped rather than scanned once per cell. + targets: match targets.iter().any(|t| t.format.is_some()) { + true => targets, + false => Vec::new(), + }, } } @@ -57,11 +71,11 @@ impl FormatPlan { pub fn uniform(fmt: &str) -> Self { Self { view: NumberFormat::parse(fmt), - by_target: HashMap::new(), + targets: Vec::new(), } } - /// The view's own answer: what a cell naming no formula target is drawn with. + /// The view's own answer: what a cell no formula owns is drawn with. pub fn view_format(&self) -> NumberFormat { self.view } @@ -69,12 +83,15 @@ impl FormatPlan { /// Asked of the *key*, never of the axes: a cell aggregating over `_Measure` /// carries no target coordinate and so keeps the view's format. pub fn at(&self, key: Option<&CellKey>) -> NumberFormat { - match key.filter(|_| !self.by_target.is_empty()) { + match key.filter(|_| !self.targets.is_empty()) { None => self.view, + // The first match in the model's order — `formula_for`'s own rule, + // an owner naming no format included. `context/code/core.md`. Some(key) => self - .by_target + .targets .iter() - .find_map(|(cat, items)| items.get(key.get(cat)?).copied()) + .find(|t| key.get(&t.category) == Some(t.target.as_str())) + .and_then(|t| t.format) .unwrap_or(self.view), } } @@ -421,6 +438,74 @@ mod plan_tests { assert_eq!(plan.at(None).value(Some(&ratio())), "0.67"); } + fn two_owner_model(first: &str, second: &str) -> Model { + let mut m = Model::new("m"); + m.add_category("Region").unwrap(); + m.add_formula(parse_formula(first, "_Measure").expect("parses")); + m.add_formula(parse_formula(second, "Region").expect("parses")); + m + } + + /// A key both formulas claim: `_Measure=Margin` and `Region=North`. + fn contested() -> CellKey { + CellKey::new(vec![ + ("_Measure".to_string(), "Margin".to_string()), + ("Region".to_string(), "North".to_string()), + ]) + } + + /// improvise-e0ex — see `context/test-evidence/core.md`. + #[test] + fn the_formula_that_owns_a_cell_names_its_format_even_when_it_names_none() { + // Owner first, and it names NO format: the view answers, and the later + // formula's `.4` must not reach a cell whose value it does not produce. + let owner_unformatted = two_owner_model( + "Margin = Profit / Revenue", + "North = Profit / Revenue FORMAT .4", + ); + let view = View::new("Default"); + assert_eq!( + FormatPlan::of(&owner_unformatted, &view) + .at(Some(&contested())) + .value(Some(&ratio())), + "0.67", + "a later formula's format overrode the formula that owns the cell" + ); + + // The same two formulas the other way round: now the owner names `.4`. + let owner_formatted = two_owner_model( + "Margin = Profit / Revenue FORMAT .4", + "North = Profit / Revenue", + ); + assert_eq!( + FormatPlan::of(&owner_formatted, &view) + .at(Some(&contested())) + .value(Some(&ratio())), + "0.6667", + "the owner's own format must win" + ); + } + + /// improvise-e0ex — see `context/test-evidence/core.md`. + #[test] + fn the_owner_is_the_one_model_order_names_and_not_the_one_a_hash_does() { + let model = two_owner_model( + "Margin = Profit / Revenue", + "North = Profit / Revenue FORMAT .4", + ); + let view = View::new("Default"); + // A `HashMap` gets a fresh `RandomState` per process, so one run cannot + // see hash order (gotcha 18); the plan is rebuilt to make that the point. + for _ in 0..5 { + assert_eq!( + FormatPlan::of(&model, &view) + .at(Some(&contested())) + .value(Some(&ratio())), + "0.67" + ); + } + } + /// improvise-e0ex — see `context/test-evidence/core.md`. #[test] fn a_formula_naming_no_format_leaves_its_target_on_the_views() { diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 00e0843d..9cafca1c 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -586,6 +586,17 @@ impl GridLayout { Some(CellKey::new(coords)) } + /// The coordinates a COLUMN answers for once its rows are aggregated away: + /// the page/filter constraints and the column's own tuple (core.md §6). + pub fn column_key(&self, col: usize) -> Option { + let col_item = data_items(&self.col_items).nth(col)?; + let mut coords = page_constraints(&self.page_coords); + for (cat, item) in self.col_cats.iter().zip(col_item.iter()) { + coords.push((cat.clone(), item.clone())); + } + Some(CellKey::new(coords)) + } + /// Visual index of the nth data row (skipping group headers). pub fn data_row_to_visual(&self, data_row: usize) -> Option { data_index_to_visual(&self.row_items, data_row) From 260f6e0b48fea51600656453b54d930a758f5bf6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 23:59:35 -0700 Subject: [PATCH 3/3] test: the page-menu spec reads the demo under the decimal default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `harness/tests/pages.spec.js` arrived on `main` with #123 while this branch was open, and asserts `toHaveText("20,300")` against `web/demo.improv`. The merge was clean and the combination was wrong — design-principles §7's third case exactly: `git merge-tree` answers "can git combine these texts", not "does the combination do what both sides meant". CI caught it on the merge ref, which is the only place the two sides had ever been run together. The assertion is unchanged in what it discriminates: it exists so that a menu moving only the label fails, and `"20,300.00"` separates Q4 from Q1's `"18,400.00"` exactly as the old literal did. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- harness/tests/pages.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/harness/tests/pages.spec.js b/harness/tests/pages.spec.js index 90fd338a..26f72d1b 100644 --- a/harness/tests/pages.spec.js +++ b/harness/tests/pages.spec.js @@ -58,7 +58,7 @@ test.describe("a page axis can be picked from a menu", () => { // Espresso Revenue is 18,400 in Q1 and 20,300 in Q4, so a menu that moved // only the label would leave this unchanged. await expect(firstCell).not.toHaveText(q1 ?? ""); - await expect(firstCell).toHaveText("20,300"); + await expect(firstCell).toHaveText("20,300.00"); }); // The menu is operable without a mouse, and while it is open it owns the