Review fixes for #2393, #2394, #2397 and #2399

- PMT: take rate / m first so (pv + fv) / m can't overflow for a tiny rate (PMT(1e-300,10,1e12)).
- Keytips probe: an empty params object keys like no params.
- Number Stored as Text check reads the same accounting parentheses as typed entry.
- Lookups: a NaN number falls back to the general comparison instead of comparing equal.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Brandon Thomas
2026-10-24 02:12:07 -04:00
co-authored by Claude Opus 5.5
parent beda2a44dc
commit 72c1d57d1e
4 changed files with 21 additions and 7 deletions
+16 -4
View File
@@ -54,8 +54,10 @@ pub fn parse_number_text_in(s: &str, sys: DateSystem) -> Option<f64> {
/// fractions kept as text are not flagged, as in Excel. /// fractions kept as text are not flagged, as in Excel.
pub fn number_stored_as_text(s: &str) -> Option<f64> { pub fn number_stored_as_text(s: &str) -> Option<f64> {
let t = s.trim(); let t = s.trim();
match t.strip_prefix('(').and_then(|x| x.strip_suffix(')')) { // The same parentheses typed entry reads, accounting forms included: `$(5)`, `(5)%`.
Some(inner) => parse_plain(inner.trim()).map(|(n, _)| -n), match parenthesized(t) {
Some(inner) if inner.strip_prefix('$').map_or(inner.as_str(), str::trim_start).starts_with(['-', '+', '(']) => None,
Some(inner) => parse_plain(&inner).map(|(n, _)| -n),
None => parse_plain(t).map(|(n, _)| n), None => parse_plain(t).map(|(n, _)| n),
} }
} }
@@ -593,11 +595,21 @@ mod tests {
#[test] #[test]
fn numbers_stored_as_text() { fn numbers_stored_as_text() {
for (text, n) in [("123", 123.0), (" 42 ", 42.0), ("-1,000.5", -1000.5), ("$3", 3.0), ("50%", 0.5), ("1E3", 1000.0), ("(100)", -100.0)] { for (text, n) in [
("123", 123.0),
(" 42 ", 42.0),
("-1,000.5", -1000.5),
("$3", 3.0),
("50%", 0.5),
("1E3", 1000.0),
("(100)", -100.0),
("$(5)", -5.0),
("(5)%", -0.05),
] {
assert_eq!(number_stored_as_text(text), Some(n), "{text}"); assert_eq!(number_stored_as_text(text), Some(n), "{text}");
} }
// Dates, times, fractions, words and blanks are not flagged. // Dates, times, fractions, words and blanks are not flagged.
for text in ["3/4/2026", "10:30", "1 1/2", "abc", "12abc", "", " ", "TRUE", "#N/A", "((1))"] { for text in ["3/4/2026", "10:30", "1 1/2", "abc", "12abc", "", " ", "TRUE", "#N/A", "((1))", "(-5)", "$(5)%", "(1/2)"] {
assert_eq!(number_stored_as_text(text), None, "{text}"); assert_eq!(number_stored_as_text(text), None, "{text}");
} }
} }
+3 -1
View File
@@ -30,7 +30,8 @@ fn pmt_calc(rate: f64, nper: f64, pv: f64, fv: f64, typ: f64) -> f64 {
let k = 1.0 + rate * typ; let k = 1.0 + rate * typ;
if m > 0.0 { if m > 0.0 {
// (fv + pv*p) / (p - 1) = pv + (pv + fv) / (p - 1), without the cancellation in p - 1. // (fv + pv*p) / (p - 1) = pv + (pv + fv) / (p - 1), without the cancellation in p - 1.
-(pv + (pv + fv) / m) * rate / k // rate / m (about 1 / nper for a tiny rate) is taken first so (pv + fv) / m can't overflow.
-(pv * rate + (pv + fv) * (rate / m)) / k
} else if m > -0.5 { } else if m > -0.5 {
// p near 1 from below: fv + pv*p = (pv + fv) + pv*m keeps the small terms that rounding p would drop. // p near 1 from below: fv + pv*p = (pv + fv) + pv*m keeps the small terms that rounding p would drop.
-(rate * (pv + fv + pv * m)) / (k * m) -(rate * (pv + fv + pv * m)) / (k * m)
@@ -1837,6 +1838,7 @@ mod tests {
assert_eq!(ev("PMT", vec![n(5.0), n(1000.0), n(100.0)]), Value::Number(-500.0)); assert_eq!(ev("PMT", vec![n(5.0), n(1000.0), n(100.0)]), Value::Number(-500.0));
assert_eq!(ev("PMT", vec![n(1e-300), n(10.0), n(100.0)]), Value::Number(-10.0)); assert_eq!(ev("PMT", vec![n(1e-300), n(10.0), n(100.0)]), Value::Number(-10.0));
close_tol(ev("PMT", vec![n(1e-12), n(10.0), n(100.0)]), -10.000000000055001, 1e-15); close_tol(ev("PMT", vec![n(1e-12), n(10.0), n(100.0)]), -10.000000000055001, 1e-15);
close_tol(ev("PMT", vec![n(1e-300), n(10.0), n(1e12)]), -1e11, 1e-15);
close_tol(ev("PMT", vec![n(-0.999), n(5.0), n(100.0)]), -9.990_000_000_000_092e-14, 1e-15); close_tol(ev("PMT", vec![n(-0.999), n(5.0), n(100.0)]), -9.990_000_000_000_092e-14, 1e-15);
close_tol(ev("PMT", vec![n(0.05), n(-10.0), n(100.0)]), 7.950_457_496_545_667, 1e-15); close_tol(ev("PMT", vec![n(0.05), n(-10.0), n(100.0)]), 7.950_457_496_545_667, 1e-15);
close_tol(ev("PMT", vec![n(-1e-17), n(1.0), n(1e18), n(-1e18)]), 10.0, 1e-9); close_tol(ev("PMT", vec![n(-1e-17), n(1.0), n(1e18), n(-1e18)]), 10.0, 1e-9);
+1 -1
View File
@@ -31,7 +31,7 @@ fn needle(args: &[Arg], i: usize) -> R<Value> {
/// The same holds for `=100+1E-14` against 100, and in every XLOOKUP / XMATCH mode. /// The same holds for `=100+1E-14` against 100, and in every XLOOKUP / XMATCH mode.
fn lookup_order(a: &Value, b: &Value) -> Ordering { fn lookup_order(a: &Value, b: &Value) -> Ordering {
match (a, b) { match (a, b) {
(Value::Number(x), Value::Number(y)) => x.partial_cmp(y).unwrap_or(Ordering::Equal), (Value::Number(x), Value::Number(y)) => x.partial_cmp(y).unwrap_or_else(|| compare(a, b)),
_ => compare(a, b), _ => compare(a, b),
} }
} }
+1 -1
View File
@@ -58,7 +58,7 @@ struct Entry {
impl Entry { impl Entry {
/// The probe key: a command and the params its keytip passes. /// The probe key: a command and the params its keytip passes.
fn probe_key(&self) -> String { fn probe_key(&self) -> String {
if self.params.is_empty() { self.target.clone() } else { format!("{} {}", self.target, self.params) } if self.params.is_empty() || self.params == "{}" { self.target.clone() } else { format!("{} {}", self.target, self.params) }
} }
} }