improvise-711n: a trailing conversion letter no longer erases the precision
`parse_number_format` read the precision as `fmt[i+1..].parse::<u8>()` over
everything after the last `.`, so `,.2f` — the printf spelling the repo-map's
own `.improv` example carries — failed to parse and fell through to zero
decimals, silently, in the TUI, the browser projection and CSV export.
The precision is now the leading digit run after the last `.`, with any
conversion letter ignored. The spellings that are legal are stated once, in
`context/code/core.md`, along with why the trailing `f` is accepted rather than
refused (the grammar takes the format line as an opaque `rest_of_line`, so
refusing it rejects documents this repo already writes) and why the reading is
printf's rather than Excel's (`.00` is the spelling the two disagree on).
`export_csv_quotes_commas_per_rfc4180` asserted the value `starts_with("1,200")`
against a `,.2f` fixture, so it could not tell two decimals from none. It is now
an exact equality, and reverting the fix fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -641,3 +641,45 @@ targets, which is what the column list shows anyway.
|
||||
And the list is **not** filtered to visible items. Hidden items are still cycled,
|
||||
because that is what the TUI has always done, and making the browser differ would
|
||||
be the divergence rather than the fix. Changing it is a change to both, together.
|
||||
|
||||
## The number format — `format.rs` (improvise-711n)
|
||||
|
||||
A view's `number_format` is a **printf-style picture, and only that**. A `,`
|
||||
anywhere turns on thousands grouping; a `.` introduces the precision, which is
|
||||
the **leading digit run after the last `.`**; anything following those digits —
|
||||
a conversion letter — is ignored. So `,.0`, `,.2`, `.2`, `,.2f` and `%.2f` all
|
||||
name what they look like they name.
|
||||
|
||||
**Why the trailing `f` is accepted rather than refused.** It round-trips into
|
||||
documents today and always has. `improv.pest` spells the line
|
||||
`format_line = { "format: " ~ rest_of_line ~ NEWLINE ~ blank_lines }` — the
|
||||
grammar takes the format string as an **opaque line**, so `format: ,.2f` is a
|
||||
document the grammar accepts; `:set-number-format` stores whatever the user
|
||||
typed verbatim (`cmd/effect_cmds.rs`), and `format_md` writes `view.number_format`
|
||||
back out unchanged. Refusing the spelling at load would therefore reject
|
||||
documents this repo's own grammar admits and its own `.improv` example spells,
|
||||
to no benefit: by gotcha 8 the format is display-only, so misreading it costs a
|
||||
wrong pixel and can never cost a wrong number. A refusal here buys nothing and
|
||||
costs a document.
|
||||
|
||||
**Why printf and not Excel.** The two conventions disagree on exactly the
|
||||
spelling that distinguishes them: `.00` is *precision zero* to printf and *two
|
||||
places* to Excel, so a parser that tried to honour both would have to guess. One
|
||||
is picked, and it is the one every spelling in this repository already uses.
|
||||
Consequences worth stating rather than discovering: `0.00` reads as
|
||||
`(false, 0)`, and an Excel picture such as `#,##0.00` reads as
|
||||
`(true, 0)` — grouped, no decimals.
|
||||
|
||||
**Nothing is validated, deliberately.** A spelling the rule cannot read names no
|
||||
precision and formats with none; there is no error channel and no `Result`,
|
||||
because the reader is display-only and four call sites would have to thread one.
|
||||
|
||||
**What the bug was, and why six tests sat on top of it.** The precision was read
|
||||
as `fmt[i+1..].parse::<u8>()` over the *whole* tail, so `"2f"` failed to parse
|
||||
and fell through to `unwrap_or(0)` — `,.2f` displayed **zero** decimals, in the
|
||||
TUI, in the browser projection and in CSV export. `",.2f"` is the spelling six
|
||||
tests in this workspace reach for, and none could see it: they assert the stored
|
||||
*string* round-trips, or — in `export_csv_quotes_commas_per_rfc4180` — that the
|
||||
rendered value `starts_with("1,200")`, which `1,200` and `1,200.00` both satisfy.
|
||||
A prefix check over a value whose *suffix* is the thing in question answers a
|
||||
neighbouring question, which is why that one is now an exact equality.
|
||||
|
||||
@@ -611,3 +611,44 @@ narrow as it claims.
|
||||
Fault injected by deleting either emission: the corresponding test fails with
|
||||
`Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and
|
||||
`the_two_front_ends_draw_the_same_column_header` fails with it.
|
||||
|
||||
### a_trailing_conversion_letter_does_not_erase_the_precision
|
||||
|
||||
improvise-711n. `parse_number_format` read the precision as
|
||||
`fmt[i+1..].parse::<u8>().ok().unwrap_or(0)` over everything after the last `.`,
|
||||
so any trailing conversion letter made the whole suffix unparseable and the
|
||||
format fell back to **zero decimals, silently**. `format: ,.2f` — the spelling
|
||||
`context/repo-map.md`'s own `.improv` example carries, and the printf/Python
|
||||
convention a user reaches for first — displayed integers everywhere.
|
||||
|
||||
Fails before the fix with `left: (true, 0)`, `right: (true, 2)`.
|
||||
|
||||
The test also pins the reading against its nearest *wrong* neighbour. Two
|
||||
implementations both fix `,.2f`: take the leading digit run, or trim the
|
||||
trailing non-digits off the tail. They differ only on a tail with digits on both
|
||||
sides of a letter, so `".2f3"` is in the assertion. Injecting
|
||||
`s.trim_end_matches(|c: char| !c.is_ascii_digit())` in place of `leading_digits`
|
||||
fails the test at `left: (false, 0)`, `right: (false, 2)` — so the test sees the
|
||||
rule, not merely the bug.
|
||||
|
||||
### a_zero_padded_picture_string_is_read_as_printf_not_as_excel
|
||||
|
||||
The companion to the above, and the reason the fix is not "be generous". A
|
||||
forgiving reader would have to answer `.00`, where printf says precision zero
|
||||
and an Excel picture says two places. This pins the printf reading — `"0.00"` is
|
||||
`(false, 0)` — so that "accept the trailing `f`" cannot quietly grow into
|
||||
"accept anything and guess". It passes before the fix as well as after: it is
|
||||
there to stop the *next* change, not to catch this one.
|
||||
|
||||
### a_precision_too_large_to_name_reads_as_none
|
||||
|
||||
`".300"` does not fit `u8`, so it reads as no precision rather than as 300
|
||||
decimal places. Unchanged by improvise-711n and pinned so the fallback is a
|
||||
stated rule rather than an artefact of the integer type.
|
||||
|
||||
### a_format_naming_no_precision_at_all_reads_as_zero
|
||||
|
||||
`",f"` has no `.`, and `".f"` has a `.` with no digits after it. Both are zero
|
||||
decimals. Pinned because `leading_digits` returns `""` for the second, and an
|
||||
empty string reaching `parse::<u8>()` is the one path in the function with no
|
||||
digits to carry the answer.
|
||||
|
||||
@@ -459,6 +459,14 @@ real CSV reader and asserts the field *count*, which is the thing a string
|
||||
|
||||
## `improvise-io/src/import/analyzer.rs`
|
||||
|
||||
**Strengthened by improvise-711n.** The assertion on the value was
|
||||
`records[1][1].starts_with("1,200")`, and the fixture's format is `,.2f` — so
|
||||
the check could not tell `1,200` from `1,200.00` and sat green while
|
||||
`parse_number_format` threw the `.2` away. It is now an exact equality. Reverting
|
||||
the `format.rs` fix fails it with `left: "1,200"`, `right: "1,200.00"`, which is
|
||||
the cross-crate demonstration that the format bug reached a user's exported file
|
||||
and not only the screen.
|
||||
|
||||
### detect_short_year_format
|
||||
|
||||
Two-digit years are ambiguous with the four-digit format, so `%m/%d/%Y` matches
|
||||
|
||||
@@ -10,14 +10,17 @@ pub fn format_value(v: Option<&CellValue>, comma: bool, decimals: u8) -> String
|
||||
}
|
||||
}
|
||||
|
||||
/// Parse a number format string like ",.0" into (use_commas, decimal_places).
|
||||
/// The digits a precision is named by: everything up to the first non-digit.
|
||||
fn leading_digits(s: &str) -> &str {
|
||||
&s[..s.find(|c: char| !c.is_ascii_digit()).unwrap_or(s.len())]
|
||||
}
|
||||
|
||||
/// Parse a printf-style number format like ",.2f" into (use_commas, decimals).
|
||||
/// Which spellings are legal: `context/code/core.md`, "The number format".
|
||||
pub fn parse_number_format(fmt: &str) -> (bool, u8) {
|
||||
let comma = fmt.contains(',');
|
||||
let decimals = fmt
|
||||
.rfind('.')
|
||||
.and_then(|i| fmt[i + 1..].parse::<u8>().ok())
|
||||
.unwrap_or(0);
|
||||
(comma, decimals)
|
||||
let precision = fmt.rsplit_once('.').map_or("", |(_, tail)| leading_digits(tail));
|
||||
(comma, precision.parse::<u8>().unwrap_or(0))
|
||||
}
|
||||
|
||||
/// Round half away from zero (the "normal" rounding people expect).
|
||||
@@ -100,6 +103,38 @@ mod tests {
|
||||
assert_eq!(parse_number_format(",.1.3"), (true, 3));
|
||||
}
|
||||
|
||||
/// Bug improvise-711n: a trailing conversion letter made the whole suffix
|
||||
/// unparseable. Evidence: `context/test-evidence/core.md`.
|
||||
#[test]
|
||||
fn a_trailing_conversion_letter_does_not_erase_the_precision() {
|
||||
assert_eq!(parse_number_format(",.2f"), (true, 2));
|
||||
assert_eq!(parse_number_format(".2f"), (false, 2));
|
||||
assert_eq!(parse_number_format("%.2f"), (false, 2));
|
||||
// The precision is the LEADING digit run, not the tail with its
|
||||
// non-digits trimmed off — these two readings differ here.
|
||||
assert_eq!(parse_number_format(".2f3"), (false, 2));
|
||||
}
|
||||
|
||||
/// One convention, not two: `.00` is printf's "precision zero", never
|
||||
/// Excel's "two places". Evidence: `context/test-evidence/core.md`.
|
||||
#[test]
|
||||
fn a_zero_padded_picture_string_is_read_as_printf_not_as_excel() {
|
||||
assert_eq!(parse_number_format("0.00"), (false, 0));
|
||||
assert_eq!(parse_number_format(",.0f"), (true, 0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_precision_too_large_to_name_reads_as_none() {
|
||||
assert_eq!(parse_number_format(".300"), (false, 0));
|
||||
assert_eq!(parse_number_format(",.999f"), (true, 0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_format_naming_no_precision_at_all_reads_as_zero() {
|
||||
assert_eq!(parse_number_format(",f"), (true, 0));
|
||||
assert_eq!(parse_number_format(".f"), (false, 0));
|
||||
}
|
||||
|
||||
// ── format_f64 basic ───────────────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -139,11 +139,9 @@ mod tests {
|
||||
assert_eq!(r.len(), 2, "comma split a field: {r:?}");
|
||||
}
|
||||
assert_eq!(&records[1][0], "Smith, Jr.");
|
||||
assert!(
|
||||
records[1][1].starts_with("1,200"),
|
||||
"expected comma-grouped value, got: {:?}",
|
||||
&records[1][1]
|
||||
);
|
||||
// Exact, not `starts_with`: the fixture asks for `.2f`, and a prefix
|
||||
// check cannot tell two decimals from none (improvise-711n).
|
||||
assert_eq!(&records[1][1], "1,200.00");
|
||||
}
|
||||
|
||||
/// Bug improvise-iot6: `render_csv` built its layout with `GridLayout::new`,
|
||||
|
||||
Reference in New Issue
Block a user