amadan

oficina / sheets Public

Number formatting: add coverage (rounding, errors, blanks, dates, unsupported fallback) and fix dead-code + literal-section sign bugs found in self-review

Paul Campbell 6df85e58cb3fce4bf7c5cf7882af8849b55bbe89 parent 0107201
5 files changed, +210 −27
  • internal/sheets/format/dates.go +6 −8
  • internal/sheets/format/format.go +19 −12
  • internal/sheets/format/format_test.go +1 −1
  • internal/sheets/format/rounding_test.go +160 −0
  • internal/sheets/format/section.go +24 −6

Intent

full ledger: core

diff --git a/internal/sheets/format/dates.go b/internal/sheets/format/dates.go
index 94a4976..514ce41 100644
--- a/internal/sheets/format/dates.go
+++ b/internal/sheets/format/dates.go
@@ -9,14 +9,12 @@ import (
// excelEpoch mirrors calc/fn_date.go's constant of the same name and
// value: day zero in spreadsheet date-serial numbering (Dec 30, 1899).
-// format cannot reach calc's unexported var (and wouldn't want the
-// coupling if it could — this package's only allowed import from calc
-// is Value), so the value is restated here; it must stay in lockstep
-// with calc's for a date serial to render as the calendar date that
-// produced it. See calc/fn_date.go's comment for why this particular
-// epoch (one day off from the "real" Dec 31, 1899) is the right one:
-// it reproduces Excel's date-serial numbering using ordinary calendar
-// arithmetic, no fake-leap-day special case required.
+// format cannot reach calc's unexported var (its only allowed import
+// from calc is Value), so the value is restated here; it must stay in
+// lockstep with calc's, or a date serial stops rendering as the
+// calendar date that produced it. See calc/fn_date.go for why this
+// epoch (one day off "real" Dec 31, 1899) reproduces Excel's numbering
+// with ordinary calendar arithmetic, no fake-leap-day case needed.
var excelEpoch = time.Date(1899, time.December, 30, 0, 0, 0, 0, time.UTC)
type dateTokKind int
diff --git a/internal/sheets/format/format.go b/internal/sheets/format/format.go
index 3d24dfb..aa30868 100644
--- a/internal/sheets/format/format.go
+++ b/internal/sheets/format/format.go
@@ -103,9 +103,9 @@ func displayNumber(num float64, numFmt string) string {
code = "General"
}
sections := splitSections(code)
- sec, negate, abs := chooseNumberSection(sections, num)
+ sec, negate := chooseNumberSection(sections, num)
val := num
- if abs && val < 0 {
+ if val < 0 {
val = -val
}
out, ok := renderSection(sec, val)
@@ -123,27 +123,34 @@ func displayNumber(num float64, numFmt string) string {
}
// chooseNumberSection picks which section applies to num, and whether an
-// automatic "-" belongs in front of it. See the package doc for the
-// section-count rules this implements.
-func chooseNumberSection(sections []string, num float64) (section string, negate, useAbs bool) {
+// automatic "-" belongs in front of it. Every branch renders against
+// abs(num) — see the package doc's section-count rules: a single-section
+// format gets an automatic sign over the absolute value, while two or
+// more sections supply their own negative styling (over the absolute
+// value too) with no automatic sign, exactly as Excel does. The
+// automatic sign only applies when that single section actually renders
+// the number (numeric or date placeholders) — a literal-only section
+// like "n/a" is a fixed label, not a number with its sign stripped, so
+// it gets no automatic "-" either.
+func chooseNumberSection(sections []string, num float64) (section string, negate bool) {
switch len(sections) {
case 0:
- return "General", num < 0, true
+ return "General", num < 0
case 1:
- return sections[0], num < 0, true
+ return sections[0], num < 0 && rendersNumber(sections[0])
case 2:
if num < 0 {
- return sections[1], false, true
+ return sections[1], false
}
- return sections[0], false, true
+ return sections[0], false
default: // 3 or more (a 4th section, if present, is for text only)
switch {
case num < 0:
- return sections[1], false, true
+ return sections[1], false
case num == 0:
- return sections[2], false, true
+ return sections[2], false
default:
- return sections[0], false, true
+ return sections[0], false
}
}
}
diff --git a/internal/sheets/format/format_test.go b/internal/sheets/format/format_test.go
index 1869be5..4a04524 100644
--- a/internal/sheets/format/format_test.go
+++ b/internal/sheets/format/format_test.go
@@ -6,7 +6,7 @@ import (
"amadan.net/oficina/sheets/internal/sheets/calc"
)
-func numValue(n float64) calc.Value { return calc.Value{Kind: calc.KindNum, Num: n} }
+func numValue(n float64) calc.Value { return calc.Value{Kind: calc.KindNum, Num: n} }
func errValue(e calc.ErrKind) calc.Value { return calc.Value{Kind: calc.KindErr, Err: e} }
func TestDisplay(t *testing.T) {
diff --git a/internal/sheets/format/rounding_test.go b/internal/sheets/format/rounding_test.go
new file mode 100644
index 0000000..995f22b
--- /dev/null
+++ b/internal/sheets/format/rounding_test.go
@@ -0,0 +1,160 @@
+package format
+
+import (
+ "testing"
+ "time"
+
+ "amadan.net/oficina/sheets/internal/sheets/calc"
+)
+
+// strconv.FormatFloat/fmt round half-to-even ("banker's rounding"):
+// 0.005 at "0.00" comes out "0.00" in Go, but Excel (and every other
+// spreadsheet) rounds half-away-from-zero and shows "0.01". This test
+// pins that Display follows Excel, not Go's default — see roundDecimal
+// in numeric.go, which works from a decimal-string representation
+// specifically to make this the natural outcome rather than a special
+// case bolted on top of float rounding.
+func TestDisplayRoundsHalfAwayFromZero(t *testing.T) {
+ for _, tc := range []struct {
+ num float64
+ want string
+ }{
+ {0.005, "0.01"},
+ {0.015, "0.02"},
+ } {
+ if got := Display(numValue(tc.num), "0.00"); got != tc.want {
+ t.Errorf("Display(%v, %q) = %q, want %q", tc.num, "0.00", got, tc.want)
+ }
+ }
+}
+
+// Every ErrKind must display as its Excel text under any number format,
+// not just #DIV/0! under one format — the single case the brief pins.
+func TestDisplayEveryErrorIgnoresNumberFormat(t *testing.T) {
+ kinds := []calc.ErrKind{
+ calc.ErrDiv0, calc.ErrValue, calc.ErrRef, calc.ErrName,
+ calc.ErrNum, calc.ErrNA, calc.ErrCircular,
+ }
+ for _, k := range kinds {
+ want := k.String()
+ for _, numFmt := range []string{"General", "0.00", "#,##0.00", "0%", "yyyy-mm-dd"} {
+ if got := Display(errValue(k), numFmt); got != want {
+ t.Errorf("Display(%v, %q) = %q, want %q", k, numFmt, got, want)
+ }
+ }
+ }
+}
+
+// KindEmpty is the zero value of ValueKind, so a Value{} — an untouched
+// cell — must render as "" under any format, never as the number "0".
+func TestDisplayBlankIsEmptyString(t *testing.T) {
+ for _, numFmt := range []string{"General", "0.00", "#,##0.00", "0%", `"€"#,##0.00`, "yyyy-mm-dd"} {
+ if got := Display(calc.Value{}, numFmt); got != "" {
+ t.Errorf("Display(blank, %q) = %q, want \"\"", numFmt, got)
+ }
+ }
+}
+
+func TestDisplayBoolIgnoresNumberFormat(t *testing.T) {
+ trueV := calc.Value{Kind: calc.KindBool, Bool: true}
+ falseV := calc.Value{Kind: calc.KindBool, Bool: false}
+ if got := Display(trueV, "0.00"); got != "TRUE" {
+ t.Errorf("Display(TRUE, %q) = %q, want TRUE", "0.00", got)
+ }
+ if got := Display(falseV, "0.00"); got != "FALSE" {
+ t.Errorf("Display(FALSE, %q) = %q, want FALSE", "0.00", got)
+ }
+}
+
+// A text value is not reformatted by a numeric format code (matching
+// Excel: a number format only applies to a text section, the 4th of
+// up to four ';'-separated sections, absent here).
+func TestDisplayTextPassesThroughNumericFormat(t *testing.T) {
+ v := calc.Value{Kind: calc.KindStr, Str: "hello"}
+ if got := Display(v, "0.00"); got != "hello" {
+ t.Errorf("Display(%q, %q) = %q, want %q", "hello", "0.00", got, "hello")
+ }
+}
+
+func TestDisplayTextSection(t *testing.T) {
+ v := calc.Value{Kind: calc.KindStr, Str: "n/a"}
+ if got := Display(v, `0;0;0;"got: "@`); got != "got: n/a" {
+ t.Errorf("Display with text section = %q, want %q", got, "got: n/a")
+ }
+}
+
+// A negative number under a single-section format gets an automatic
+// "-" over the absolute value's rendering.
+func TestDisplayNegativeSingleSectionAutoSign(t *testing.T) {
+ if got := Display(numValue(-1234.5), "#,##0.00"); got != "-1,234.50" {
+ t.Fatalf("Display(-1234.5, %q) = %q, want %q", "#,##0.00", got, "-1,234.50")
+ }
+}
+
+// A two-or-more-section format supplies its own negative styling with
+// NO automatic sign — matching Excel, where "0.00;(0.00)" is how you
+// spell accounting-style parenthesized negatives.
+func TestDisplayNegativeSectionNoAutoSign(t *testing.T) {
+ if got := Display(numValue(-5), "0.00;(0.00)"); got != "(5.00)" {
+ t.Fatalf("Display(-5, %q) = %q, want %q", "0.00;(0.00)", got, "(5.00)")
+ }
+}
+
+// A section with no digit or date placeholders is a fixed literal —
+// punctuation in it (not a bare letter) is not an unsupported construct,
+// it just displays the same text regardless of value.
+func TestDisplayLiteralOnlySection(t *testing.T) {
+ if got := Display(numValue(5), `"-"`); got != "-" {
+ t.Errorf(`Display(5, "-") = %q, want "-"`, got)
+ }
+ if got := Display(numValue(-99), `"n/a"`); got != "n/a" {
+ t.Errorf(`Display(-99, "n/a") = %q, want "n/a"`, got)
+ }
+}
+
+// An unsupported construct (here '_' — Excel's space-padding fill
+// character) falls the WHOLE value back to General rather than
+// mangling the output partially.
+func TestDisplayUnsupportedFallsBackToGeneral(t *testing.T) {
+ for _, numFmt := range []string{"0.00_)", "q", "0.00E+00", "# ?/?"} {
+ got := Display(numValue(1234.5678), numFmt)
+ want := Display(numValue(1234.5678), "General")
+ if got != want {
+ t.Errorf("Display(1234.5678, %q) = %q, want General's %q", numFmt, got, want)
+ }
+ }
+}
+
+// serialFor mirrors calc's dateSerial (fn_date.go) and this package's
+// excelEpoch (dates.go) independently, so a date test here pins the
+// same convention rather than assuming it.
+func serialFor(y int, mo time.Month, d, h, mi, s int) float64 {
+ epoch := time.Date(1899, time.December, 30, 0, 0, 0, 0, time.UTC)
+ tm := time.Date(y, mo, d, h, mi, s, 0, time.UTC)
+ return tm.Sub(epoch).Hours() / 24
+}
+
+func TestDisplayDates(t *testing.T) {
+ for _, tc := range []struct {
+ name string
+ serial float64
+ numFmt string
+ want string
+ }{
+ {"iso date", serialFor(2024, time.March, 5, 0, 0, 0), "yyyy-mm-dd", "2024-03-05"},
+ {"slash date", serialFor(2024, time.March, 5, 0, 0, 0), "dd/mm/yyyy", "05/03/2024"},
+ {"abbreviated month", serialFor(2024, time.March, 5, 0, 0, 0), "mmm d, yyyy", "Mar 5, 2024"},
+ {"full month and weekday", serialFor(2024, time.March, 5, 0, 0, 0), "dddd, mmmm d, yyyy",
+ time.Date(2024, time.March, 5, 0, 0, 0, 0, time.UTC).Weekday().String() + ", March 5, 2024"},
+ {"24h time", serialFor(2024, time.March, 5, 14, 30, 9), "hh:mm:ss", "14:30:09"},
+ {"12h time PM", serialFor(2024, time.March, 5, 13, 5, 9), "hh:mm:ss AM/PM", "01:05:09 PM"},
+ {"12h time midnight", serialFor(2024, time.March, 5, 0, 0, 0), "hh:mm:ss AM/PM", "12:00:00 AM"},
+ {"date and minutes not month", serialFor(2024, time.March, 5, 14, 30, 0), "yyyy-mm-dd hh:mm", "2024-03-05 14:30"},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ if got := Display(numValue(tc.serial), tc.numFmt); got != tc.want {
+ t.Errorf("Display(%s, %q) = %q, want %q", tc.name, tc.numFmt, got, tc.want)
+ }
+ })
+ }
+}
diff --git a/internal/sheets/format/section.go b/internal/sheets/format/section.go
index 31bad7b..6e41e1e 100644
--- a/internal/sheets/format/section.go
+++ b/internal/sheets/format/section.go
@@ -62,6 +62,17 @@ func walkBareRunes(section string, fn func(byte) bool) {
}
}
+// rendersNumber reports whether section actually renders the number
+// (via "General", numeric, or date placeholders) rather than being a
+// fixed literal label — used to decide whether a single-section format
+// gets an automatic "-" for a negative value.
+func rendersNumber(section string) bool {
+ if strings.EqualFold(strings.TrimSpace(section), "General") {
+ return true
+ }
+ return detectMode(section) != modeLiteral
+}
+
// renderSection renders val (already sign-adjusted by the caller)
// through one format section. ok is false when the section uses a
// construct this package does not support, telling the caller to fall
@@ -80,15 +91,22 @@ func renderSection(section string, val float64) (string, bool) {
}
}
-// renderLiteral renders a section with no placeholders at all: the same
-// text regardless of val (e.g. a format section used purely to hide a
-// value, or to show a fixed label).
+// renderLiteral renders a section with no digit or date placeholders at
+// all: the same text regardless of val (e.g. a section used purely to
+// hide a value, or to show a fixed label like "-" or "n/a"). A bare
+// ASCII letter is still rejected — Excel reserves unquoted letters for
+// format tokens, so an unrecognized one (e.g. a stray "q") is treated
+// as an unsupported construct rather than assumed to be a literal word.
func renderLiteral(section string) (string, bool) {
var sb strings.Builder
ok := true
- walkLiteralAndPlaceholders(section, func(lit string) { sb.WriteString(lit) }, func(byte) bool {
- ok = false
- return true
+ walkLiteralAndPlaceholders(section, func(lit string) { sb.WriteString(lit) }, func(c byte) bool {
+ if isASCIILetter(c) || strings.IndexByte("?@_*", c) >= 0 {
+ ok = false
+ return true
+ }
+ sb.WriteByte(c)
+ return false
})
return sb.String(), ok
}