fix(display): Sig digit counting in formatGeneral, repeatable --with, doc nits

[arcaputo3]

Summary

• formatGeneral significant digits fix: Count significant digits (not characters) to match Excel's General format 11-digit threshold. Previously -99999999.99 (10 sig digits) incorrectly triggered scientific notation because the string was 12 chars. Now rounds via MathContext(11) instead of losing precision through Double.
• Repeatable --with flag: eval/evala now accept multiple --with flags (--with "A1=100" --with "B2=200") in addition to comma-separated syntax. Uses Opts.options (plural) with shared parseOverrides helper.
• Doc updates to SKILL.md: Document evala command, --with comma-separated syntax, --backend write-only behavior, streaming CSV formula prefix note.

Test plan

• ./mill xl-core.test — 127 tests pass
• ./mill xl-cli.test — 317 tests pass
• ./mill __.checkFormat — clean
• ./mill __.compile — clean
• Dogfood: 9 sig digits stays plain, 10 sig negative stays plain, 12 sig medium rounds, 13 sig small → scientific, 15 sig large → scientific
• Dogfood: --with "A=1,B=2" and --with "A=1" --with "B=2" produce identical results
• Dogfood: evala '=TRANSPOSE(B2:B5)' works

🤖 Generated with Claude Code

[claude]

Code Review

Summary

This PR fixes a critical bug in formatGeneral where character count was used instead of significant digit count, causing incorrect scientific notation for values like -99999999.99. It also adds support for repeatable --with flags in the CLI and updates documentation.

Strengths

1. Correct Fix: The shift from string length to actual significant digit counting is the right approach. Using MathContext(11) prevents precision loss.

2. Clean CLI Refactoring: The parseOverrides helper properly deduplicates parsing logic. Using Opts.options (plural) for repeatable flags follows decline library patterns.

3. Good Test Coverage: Manual dogfooding with edge cases and 444 tests passing.

4. Documentation: Updates to SKILL.md correctly document evala command and --with syntax.

Issues Found

1. Bug in countSignificantDigits for zero values (Medium Priority)

Line 146 has a logic error:

else s.reverse.dropWhile(_ == '0').length

This returns 0 sig digits for input "0" when it should return 1.

Suggested fix:

else
  val nonZero = s.reverse.dropWhile(_ == '0')
  if nonZero.isEmpty then 1 else nonZero.length

2. Missing unit tests (High Priority)

countSignificantDigits is private with no direct test coverage. Add tests in DisplaySpec.scala for formatGeneral edge cases:

• Exactly 11 sig digits (boundary)
• 12 sig digits (triggers rounding)
• Negative with 10 sig digits
• Zero value

Recommendations

Before merge:

1. Fix countSignificantDigits bug for zero values
2. Add test cases for formatGeneral boundaries

Post-merge:

1. Consider making countSignificantDigits testable (private[display])

Verdict

Conditional Approve - pending zero-value fix

The core logic is sound. Once the zero-value edge case is fixed and boundary tests are added, this is ready to merge. Great work on the detailed validation!

[claude]

Code Review

Summary

This PR makes three improvements: fixes the formatGeneral significant digit counting logic, adds repeatable --with flag support, and updates documentation. Overall, the changes are well-implemented with good test coverage.

---

Strengths

1. Core Bug Fix is Correct

The formatGeneral fix properly addresses the root cause:

• Before: Counted string length (12 chars for -99999999.99), incorrectly triggering scientific notation
• After: Counts actual significant digits (10), correctly stays in plain format
• Uses MathContext(11) for precision-preserving rounding instead of lossy Double conversion

2. Excellent Test Coverage

The 7 new test cases in DisplaySpec.scala comprehensively cover:

• Zero edge case
• 9-11 significant digit boundaries
• Negative numbers (the original bug case)
• Scientific notation triggers for very large/small numbers
• Rounding behavior for 12+ significant digits

3. Clean CLI Refactoring

The --with flag enhancement:

• Properly uses Opts.options (plural) for repeatability
• Extracts shared parseOverrides helper (DRY principle)
• Maintains backward compatibility with comma-separated syntax
• Applies consistently to both eval and evala commands

4. Documentation Updates

SKILL.md additions document previously undocumented features and clarify edge cases.

---

Minor Suggestions

1. Consider Adding Boundary Test for 1e-4

Test case to consider: values near the 1e-4 scientific notation threshold.

At NumFmtFormatter.scala:137, scientific notation triggers when abs < 1e-4. Consider adding:

• Test for exactly 0.0001 (should stay plain)
• Test for 0.00009999 (should trigger scientific)

2. Note on doubleValue Precision

At NumFmtFormatter.scala:137, using rounded.doubleValue after careful BigDecimal rounding reintroduces floating-point precision issues. This is acceptable for matching Excel behavior, but worth noting as a known tradeoff.

---

Security & Performance

• No security concerns: Pure functions, no IO, no validation issues
• Performance: countSignificantDigits is O(n) in string length, negligible impact
• Thread safety: All pure functions, no shared mutable state

---

Final Verdict

LGTM with minor suggestions. The core bug fix is solid, test coverage is excellent, and the CLI enhancement is well-designed.

Checklist

• Fixes reported bug correctly
• Comprehensive test coverage (7 new tests)
• Follows project style (opaque types, pure functions, total functions)
• Adheres to purity charter (no side effects, deterministic)
• No performance regressions
• Documentation updated appropriately

Recommendation: Merge after considering boundary test suggestion. Great work on the comprehensive test coverage!