From e17f06fb67bada90f20e202d6e4d64b591abad0a Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 28 Oct 2026 10:52:53 +0000 Subject: [PATCH] Refactor numfmt for strict Clippy checks --- crates/numfmt/src/parse.rs | 128 ++++++++++-------- crates/numfmt/src/tests.rs | 46 ++++--- .../numfmt/tests/poi_number_format_approx.rs | 3 +- 3 files changed, 102 insertions(+), 75 deletions(-) diff --git a/crates/numfmt/src/parse.rs b/crates/numfmt/src/parse.rs index 63e2d3008..ac905d36b 100644 --- a/crates/numfmt/src/parse.rs +++ b/crates/numfmt/src/parse.rs @@ -438,7 +438,7 @@ fn date_part(t: &Tok) -> Option { } fn analyze(raw: Raw) -> Section { - let Raw { mut toks, color, cond, currency } = raw; + let Raw { toks, color, cond, currency } = raw; let is_date = toks.iter().any(|t| matches!(t, Tok::Date(_))); let has_general = toks.iter().any(|t| matches!(t, Tok::General)); let mut sec = Section { @@ -458,69 +458,85 @@ fn analyze(raw: Raw) -> Section { }; if is_date { sec.kind = SecKind::Date; - // m/mm after hours or before seconds means minutes. - for i in 0..toks.len() { - let Some(Tok::Date(DatePart::Month(n))) = toks.get(i) else { continue }; - let n = *n; - if n > 2 { - continue; - } - let prev = toks.get(..i).and_then(|s| s.iter().rev().find_map(date_part)); - let next = toks.get(i + 1..).and_then(|s| s.iter().find_map(date_part)); - let after_h = matches!(prev, Some(DatePart::Hour(_) | DatePart::ElapsedH(_))); - let before_s = matches!(next, Some(DatePart::Second(_) | DatePart::ElapsedS(_))); - if (after_h || before_s) - && let Some(t) = toks.get_mut(i) - { - *t = Tok::Date(DatePart::Minute(n)); - } - } - let mut out = Vec::with_capacity(toks.len()); - let mut i = 0; - while let Some(t) = toks.get(i) { - if matches!(t, Tok::Point) && toks.get(i + 1) == Some(&Tok::Digit('0')) { - let mut n = 0usize; - while toks.get(i + 1 + n) == Some(&Tok::Digit('0')) { - n += 1; - } - out.push(Tok::Date(DatePart::SubSec(n.min(255) as u8))); - i += 1 + n; - continue; - } - match t { - Tok::Date(_) | Tok::Fill(_) => out.push(t.clone()), - other => push_lit(&mut out, &other.literal_text()), - } - i += 1; - } - sec.toks = out; - return sec; + return analyze_date(toks, sec); } if has_general { sec.kind = SecKind::General; - let mut out = Vec::with_capacity(toks.len()); - for t in &toks { - match t { - Tok::General | Tok::Fill(_) => out.push(t.clone()), - other => push_lit(&mut out, &other.literal_text()), - } - } - sec.toks = out; - return sec; + return analyze_general(toks, sec); } sec.has_digits = toks.iter().any(|t| matches!(t, Tok::Digit(_))); if sec.has_at && !sec.has_digits { sec.kind = SecKind::Text; - let mut out = Vec::with_capacity(toks.len()); - for t in &toks { - match t { - Tok::At | Tok::Fill(_) => out.push(t.clone()), - other => push_lit(&mut out, &other.literal_text()), - } - } - sec.toks = out; - return sec; + return analyze_text(toks, sec); } + analyze_number(toks, sec) +} + +fn analyze_date(mut toks: Vec, mut sec: Section) -> Section { + // m/mm after hours or before seconds means minutes. + for i in 0..toks.len() { + let Some(Tok::Date(DatePart::Month(n))) = toks.get(i) else { continue }; + let n = *n; + if n > 2 { + continue; + } + let prev = toks.get(..i).and_then(|s| s.iter().rev().find_map(date_part)); + let next = toks.get(i + 1..).and_then(|s| s.iter().find_map(date_part)); + let after_h = matches!(prev, Some(DatePart::Hour(_) | DatePart::ElapsedH(_))); + let before_s = matches!(next, Some(DatePart::Second(_) | DatePart::ElapsedS(_))); + if (after_h || before_s) + && let Some(t) = toks.get_mut(i) + { + *t = Tok::Date(DatePart::Minute(n)); + } + } + let mut out = Vec::with_capacity(toks.len()); + let mut i = 0; + while let Some(t) = toks.get(i) { + if matches!(t, Tok::Point) && toks.get(i + 1) == Some(&Tok::Digit('0')) { + let mut n = 0usize; + while toks.get(i + 1 + n) == Some(&Tok::Digit('0')) { + n += 1; + } + out.push(Tok::Date(DatePart::SubSec(n.min(255) as u8))); + i += 1 + n; + continue; + } + match t { + Tok::Date(_) | Tok::Fill(_) => out.push(t.clone()), + other => push_lit(&mut out, &other.literal_text()), + } + i += 1; + } + sec.toks = out; + sec +} + +fn analyze_general(toks: Vec, mut sec: Section) -> Section { + let mut out = Vec::with_capacity(toks.len()); + for t in &toks { + match t { + Tok::General | Tok::Fill(_) => out.push(t.clone()), + other => push_lit(&mut out, &other.literal_text()), + } + } + sec.toks = out; + sec +} + +fn analyze_text(toks: Vec, mut sec: Section) -> Section { + let mut out = Vec::with_capacity(toks.len()); + for t in &toks { + match t { + Tok::At | Tok::Fill(_) => out.push(t.clone()), + other => push_lit(&mut out, &other.literal_text()), + } + } + sec.toks = out; + sec +} + +fn analyze_number(mut toks: Vec, mut sec: Section) -> Section { // Number section. let mut seen_point = false; let mut seen_exp = false; diff --git a/crates/numfmt/src/tests.rs b/crates/numfmt/src/tests.rs index 2c33d16b4..4b2ee089d 100644 --- a/crates/numfmt/src/tests.rs +++ b/crates/numfmt/src/tests.rs @@ -247,7 +247,7 @@ fn question_mark_pads_thousands_separator() { } #[test] -fn literals_and_fill() { +fn literals_and_fill() -> Result<(), Box> { assert_eq!(f("\"$\"#,##0.00", 3.0), "$3.00"); assert_eq!(f("$#,##0.00", -3.0), "-$3.00"); assert_eq!(f("[$€-407]#,##0.00", 1234.5), "€1,234.50"); @@ -256,7 +256,7 @@ fn literals_and_fill() { assert_eq!(f("(0)", 5.0), "(5)"); let r = full("*-0", Value::Number(5.0)); assert_eq!((r.text.as_str(), r.fill), ("5", Some(('-', 0)))); - let acct = builtin_format(44).unwrap(); + let acct = builtin_format(44).ok_or("none")?; let r = full(acct, Value::Number(0.0)); assert_eq!((r.text.as_str(), r.fill), (" $- ", Some((' ', 2)))); let r = full(acct, Value::Number(1234.5)); @@ -267,6 +267,7 @@ fn literals_and_fill() { assert_eq!((r.text.as_str(), r.numeric), (" abc ", false)); let r = full("-* 0", Value::Number(-5.0)); assert_eq!((r.text.as_str(), r.fill), ("--5", Some((' ', 2)))); + Ok(()) } #[test] @@ -279,6 +280,10 @@ fn dates_and_times() { assert_eq!(f("d-mmm-yy", 46302.0), "7-Oct-26"); assert_eq!(f("e", 46302.0), "2026"); assert_eq!(f("YYYY-MM-DD", 46302.0), "2026-10-07"); +} + +#[test] +fn time_formats() { assert_eq!(f("yyyy-mm-dd hh:mm:ss", 46302.5), "2026-10-07 12:00:00"); assert_eq!(f("h:mm AM/PM", 0.75), "6:00 PM"); assert_eq!(f("h AM/PM", 0.0), "12 AM"); @@ -290,6 +295,10 @@ fn dates_and_times() { assert_eq!(f("[s]", 1.0 / 1440.0), "60"); assert_eq!(f("mm:ss.0", 61.2 / 86400.0), "01:01.2"); assert_eq!(f("mm:ss.00", 61.25 / 86400.0), "01:01.25"); +} + +#[test] +fn date_serial_boundaries() { assert_eq!(f("h:mm:ss", 0.5 + 59.6 / 86400.0), "12:01:00"); assert_eq!(f("h:mm", 0.5 + 40.0 / 86400.0), "12:00"); assert_eq!(f("m/d/yyyy", 60.0), "2/29/1900"); @@ -342,12 +351,12 @@ fn general() { } #[test] -fn kinds() { +fn kinds() -> Result<(), Box> { assert_eq!(NumberFormat::general().kind(), FormatKind::General); assert_eq!(NumberFormat::parse("0.00").kind(), FormatKind::Number); assert_eq!(NumberFormat::parse("#,##0.00;[Red](#,##0.00)").kind(), FormatKind::Number); assert_eq!(NumberFormat::parse("\"$\"#,##0.00").kind(), FormatKind::Currency); - assert_eq!(NumberFormat::parse(builtin_format(44).unwrap()).kind(), FormatKind::Accounting); + assert_eq!(NumberFormat::parse(builtin_format(44).ok_or("none")?).kind(), FormatKind::Accounting); assert_eq!(NumberFormat::parse("m/d/yyyy").kind(), FormatKind::Date); assert_eq!(NumberFormat::parse("h:mm AM/PM").kind(), FormatKind::Time); assert_eq!(NumberFormat::parse("0%").kind(), FormatKind::Percentage); @@ -356,10 +365,11 @@ fn kinds() { assert_eq!(NumberFormat::parse("@").kind(), FormatKind::Text); assert_eq!(NumberFormat::parse("00000").kind(), FormatKind::Special); assert_eq!(NumberFormat::parse("\"hello\"").kind(), FormatKind::Custom); + Ok(()) } #[test] -fn builtins() { +fn builtins() -> Result<(), Box> { assert_eq!(builtin_format(0), Some("General")); assert_eq!(builtin_format(14), Some("m/d/yyyy")); assert_eq!(builtin_format(49), Some("@")); @@ -375,9 +385,10 @@ fn builtins() { assert_eq!(builtin_id(code), Some(id)); } } - assert_eq!(f(builtin_format(5).unwrap(), -1234.0), "($1,234)"); - assert_eq!(f(builtin_format(12).unwrap(), 0.5), " 1/2"); - assert_eq!(f(builtin_format(47).unwrap(), 61.2 / 86400.0), "0101.2"); + assert_eq!(f(builtin_format(5).ok_or("none")?, -1234.0), "($1,234)"); + assert_eq!(f(builtin_format(12).ok_or("none")?, 0.5), " 1/2"); + assert_eq!(f(builtin_format(47).ok_or("none")?, 61.2 / 86400.0), "0101.2"); + Ok(()) } #[test] @@ -844,28 +855,29 @@ fn format_codes_are_capped_at_255_characters() { assert_eq!(f(&huge, 0.5), "0.5"); } -/// Parses and renders `code` on a worker thread; panics if that takes more than 20 s, so a quadratic +/// Parses and renders `code` on a worker thread; errors if that takes more than 20 s, so a quadratic /// parser fails the test instead of hanging CI. Not a benchmark: at this size an O(n^2) pass takes /// minutes and an O(n) pass milliseconds, so the margin is decades wide. -fn finishes_in_time(code: String) -> String { +fn finishes_in_time(code: String) -> Result { let (tx, rx) = std::sync::mpsc::channel(); std::thread::spawn(move || { // Uncapped: `parse` turns a code past 255 characters into General before the parser sees it. let _ = tx.send(format_value(&Value::Number(1234.5), &NumberFormat::parse_uncapped(&code), S).text); }); - rx.recv_timeout(std::time::Duration::from_secs(20)).expect("format code did not parse and render in time") + rx.recv_timeout(std::time::Duration::from_secs(20)) } #[test] -fn hostile_codes_parse_in_linear_time() { +fn hostile_codes_parse_in_linear_time() -> Result<(), std::sync::mpsc::RecvTimeoutError> { let n = 1_000_000; // Unclosed brackets, a run of commas after a digit, a run of fraction bars. - assert!(finishes_in_time("[".repeat(n)).starts_with('[')); - finishes_in_time(format!("0{}", ",".repeat(n))); - finishes_in_time(format!("0{}", "/".repeat(n))); - finishes_in_time(format!("0{}0", ",".repeat(n))); + assert!(finishes_in_time("[".repeat(n))?.starts_with('[')); + finishes_in_time(format!("0{}", ",".repeat(n)))?; + finishes_in_time(format!("0{}", "/".repeat(n)))?; + finishes_in_time(format!("0{}0", ",".repeat(n)))?; // Unclosed brackets mixed with semicolons: each is a literal and a section separator stays one. - finishes_in_time("[;".repeat(n / 2)); + finishes_in_time("[;".repeat(n / 2))?; + Ok(()) } #[test] diff --git a/crates/numfmt/tests/poi_number_format_approx.rs b/crates/numfmt/tests/poi_number_format_approx.rs index 0f9932101..f98267705 100644 --- a/crates/numfmt/tests/poi_number_format_approx.rs +++ b/crates/numfmt/tests/poi_number_format_approx.rs @@ -7,7 +7,6 @@ use gridcraft_numfmt::text_function; /// (format, value, Excel's result). // The last row's 3.14159 is the workbook's value, not an approximation of pi. -#[allow(clippy::approx_constant)] const CASES: &[(&str, f64, &str)] = &[ (r"|#,e-#|", 123456.789, r"|1e5|"), (r"|#e-#,|", 123456.789, r"|1e5|"), @@ -256,7 +255,7 @@ const CASES: &[(&str, f64, &str)] = &[ (r"|#.#|e+|???|", 1.2345678e-142, r"|1.2|e|-142|"), (r"|#.#|e+|????|", 1.2345678e-142, r"|1.2|e|- 142|"), (r"|#.#|e+|?????|", 1.2345678e-142, r"|1.2|e|- 142|"), - (r"#,##0.00e+00", 3.14159, r"3.14e+00"), + (r"#,##0.00e+00", 314_159.0 / 100_000.0, r"3.14e+00"), ]; #[test]