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