Skip to content

Fix positional put smart type detection - #438

Merged
arcaputo3 merged 1 commit into
mainfrom
fix/gh-431-positional-put-detect
Jul 29, 2026
Merged

Fix positional put smart type detection#438
arcaputo3 merged 1 commit into
mainfrom
fix/gh-431-positional-put-detect

Conversation

@arcaputo3

Copy link
Copy Markdown
Contributor

Summary

  • route positional put values through the same currency, percent, ISO-date, number, and boolean detection used by batch put
  • add --no-detect for literal-text parity with batch detect:false, including the streaming path
  • preserve existing cell styling when streaming detected formats and reuse generated style IDs across ranges
  • reject unparseable strings for explicit date, datetime, and time batch formats instead of writing text with a date-like number format
  • update CLI, bundled-skill, and 1904-streaming limitation documentation

Root cause

Positional put used ValueParser.parseValue, which only recognized numbers and booleans. ISO date strings therefore became text even though batch put already used FormattedParsers.detect. The explicit date-format path also fell back to a text value while retaining the requested number format, producing cells that looked like dates but broke date formulas.

Impact

Positional ISO dates now land as numeric Excel dates with NumFmt.Date, so downstream formulas such as XIRR and EOMONTH receive typed values. Currency and percent formatting also matches batch behavior. Detection remains intentionally ISO-date-only; use --no-detect when date-like or numeric input must remain text.

The streaming implementation preserves existing font/fill/border/alignment and existing non-General number formats. Formatted range fills share generated cellXf entries instead of creating one duplicate style per cell.

Validation

  • ./mill --no-server xl-cli.checkFormat
  • ./mill --no-server xl-cli.test
  • git diff --check
  • regression coverage for CLI option parsing, in-memory and streaming date writes, literal opt-out, style preservation, bounded streaming style growth, and invalid explicit date formats

Closes #431

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review: Fix positional put smart type detection (#431)

Reviewed the full diff plus the surrounding code paths (FormattedParsers.detect, Sheet.putSingle/mergeStyles, StylePatcher.addStyle, StreamingTransform, and readExistingWorksheetMetadata). This is a well-scoped, high-quality change: the fix reuses the existing FormattedParsers.detect rather than reimplementing detection, the streaming style-reuse logic faithfully mirrors the in-memory Sheet.put(Formatted) semantics, the docs/skill are updated in lockstep, and the tests cover the important round-trips. Nice work.

A few things worth considering before merge:

Correctness / behavior

  1. Explicit time format can no longer accept a time-of-day string (minor regression). In BatchParser, the explicit date | datetime | time branch now hard-errors when FormattedParsers.parseDate fails. But parseDate only accepts ISO YYYY-MM-DD (it does LocalDate.parse and returns midnight), so an input like {"op":"put","format":"time","value":"14:30"} previously fell back to Text and now fails the whole batch. The stricter rejection is clearly right for date/datetime, but for time it means the only accepted input is a bare date (which renders as 00:00), so legitimate-looking time strings now error. Worth either documenting that time requires a date input or leaving time with the softer fallback.

  2. Breaking behavior change for positional put (intended, but flag for release notes). Values like 50%, $1,234.56, and 2024-03-15 that previously landed as text now become typed numbers/dates. This is the point of the fix and --no-detect is a clean opt-out, but it's a semantics change for existing scripts (e.g. codes/IDs that look like ISO dates) and should be called out in the changelog.

Performance

  1. Formatted streaming puts now make two full worksheet passes. For a formatted --stream put, scanExistingStyles walks the entire worksheet to recover the target cell's existing style id, and then transformWithMetadata walks it again to apply the patch. Memory stays O(1), but read time roughly doubles on very large files. Since --stream exists specifically for huge files, it may be worth folding the existing-style lookup into the transform pass later. Not a regression (formatted streaming puts weren't supported before), just a note. Plain-value puts correctly short-circuit (readExistingWorksheetMetadata returns empty when no col/row ops, and scanExistingStyles is skipped when there are no formatted refs), so the common path is unaffected.

Test coverage

  1. The "preserve existing explicit format" branch is untested. The streaming tests exercise the merge branch (existing General + bold, keep bold, apply Date), the range-dedup path, and --no-detect, but nothing covers the else (existingStyle, existingStyleId) branch where the target cell already has a non-General numFmt (e.g. a Currency-formatted cell receiving a date value should keep Currency and reuse its style id). A test there would lock in the intended Sheet.put(Formatted) parity.

  2. Positional-put detection is only tested for ISO dates. Currency, percent, and boolean detection for positional put rely on FormattedParsers.detect (which has its own tests), but a one-line assertion each for put A1 "\$100" and put A1 "50%" would guard against future regressions in the wiring.

Nit

  1. In ValueParser.parsePutValue, the case Formatted(CellValue.Text(_), numFmt) reuses numFmt from detect, but detect only ever returns NumFmt.General for Text. Using NumFmt.General directly would make the intent clearer and avoid implying a text value could carry a non-General format.

Overall this looks solid and ready to merge once the time-format edge case (#1) is resolved or documented; the rest are follow-up-friendly.

@arcaputo3
arcaputo3 marked this pull request as ready for review July 29, 2026 15:21
@arcaputo3
arcaputo3 merged commit 4075cf4 into main Jul 29, 2026
4 checks passed
arcaputo3 added a commit that referenced this pull request Jul 29, 2026
…mics, sheet lifecycle, comment/sheetFormatPr round-trip (#440)

* fix(core): remap DefinedName.localSheetId across sheet order mutations

removeAt shifts scopes above the removal down and drops names scoped to
the removed sheet (Excel's delete-sheet semantics); insertAt shifts
scopes at/above the insertion point up; reorder applies the full
old-to-new permutation. Verbatim (unmodelable) print names in
metadata.definedNames follow their sheet like any scoped name; the
modeled PrintNames.fromSheets re-derivation path is untouched and now
pinned by test.

Refs #434

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* feat(core): Sheet.named — the documented dynamic-name factory

Sheet.apply is transparent inline: a literal specializes to Sheet, a
dynamic String to XLResult[Sheet], with nothing at the call site
hinting the return type changed (cost a field build three edit
cycles). Sheet.named(name) spells the XLResult in a plain non-inline
signature and is now the documented path for runtime names; the union
apply's scaladoc, the scripting guide, and the xl-scripting skill
(quick ref + literal-vs-dynamic pattern + gotcha, API.md table) steer
dynamic callers there. The inline apply and the SheetLiteral macro are
untouched; version pins stay 0.16.0.

Refs #420

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(cli): batch put values[] threads op-level format into every element

The values[] arm of batch put constructed BatchOp.PutValues without
reading the op-level "format" field, silently writing General cells
while the single-value arm and every putf variant applied the numFmt
("format" sits in knownPutProps, so no unknown-prop warning fired
either).

parseJsonValue now takes the parsed op-level format and applies it
with single-put semantics per element: native numbers carry it
directly, strings are parsed according to it (including the explicit
date-format rejection path), booleans and nulls stay bare. Because
the format lands in each ParsedValue at parse time, both the
in-memory path (applyPutValues) and the streaming batch path
(StreamingWriteCommands) honor it with no further changes.

Refs #416

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ooxml): comment rewrite keeps foreign author-prefix runs and raw run properties

Two comment round-trip fidelity nits from the 0.15.0 field-gotcha audit:

- XlsxWriter no longer synthesizes a canonical author-prefix run when the
  comment text already leads with its own (first run == author + optional
  colon, any formatting) — a file-authored 'Reviewer:' run no longer comes
  back as 'Reviewer:\nReviewer: …' after a rewrite.
- Comment run serialization now prefers the preserved raw <rPr> (same
  pattern as SharedStrings), carrying <color indexed="81"/>, rgb colors,
  original fonts and <charset> through the rewrite instead of rebuilding
  a b/sz/rFont-only rPr from the Font model.

Refs #433

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(streaming): StylePatcher delegates builtin numFmt ids to canonical NumFmt tables

findOrAddNumFmt hand-rolled its own builtin-id table and diverged from
NumFmt.builtInId on three arms: Decimal emitted 4 (ThousandsDecimal,
'#,##0.00'), Percent emitted 10 (PercentDecimal, '0.00%'), Currency
emitted 44 (accounting). A cell styled through the streaming path thus
rendered differently from the same style written in-memory. Delete the
table and delegate every non-Custom arm to NumFmt.builtInId.

extractNumFmt (the read side in the same file) carried the mirror-image
divergence: only 8 ids mapped, so builtin ids 1/2/3/9/11/12 written by
the in-memory path degraded to General on getStyle, and declared
<numFmt> entries lost to the hand-rolled arms. Delegate to the DOM
StyleParser's resolution order: declared entry (verbatim Custom,
GH-404) -> NumFmt.fromId -> General.

Pins: every builtin variant lands its ECMA-376 id and round-trips
through getStyle; a streamed positional 'put A1 45.5%' (PR #438 routes
detected formats through StylePatcher) lands numFmtId 9.

Refs #408

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* feat(ooxml): emit and read sheetFormatPr for sheet default row height/width

Sheet.defaultRowHeight/defaultColumnWidth were read/model-only: neither
writer backend emitted <sheetFormatPr>, so a from-scratch book could not
set the sheet-default row height (house grids want 12.75) or default
column width, and the reader never populated the fields.

- mergeSheetFormatPrElem (WorksheetHelpers): overlays the modeled
  defaults onto any preserved <sheetFormatPr> (unmodeled attrs like
  baseColWidth/outlineLevelRow ride through), identity fast-path keeps
  an unchanged source element verbatim, model-driven heights get the
  customHeight="1" companion (ECMA-376 18.3.1.81), and a fresh
  width-only element backfills the REQUIRED defaultRowHeight ("15").
- OoxmlWorksheet.fromDomainWithMetadata wires the merge on both the
  preserved and fresh branches (CT_Worksheet slot before <cols>).
- DirectSaxEmitter emits the same element on the streaming path.
- XlsxReader.convertToDomainSheet populates the model fields from
  <sheetFormatPr> so read-modify-write keeps them.
- SheetFormatPrRoundTripSpec: round-trip on both backends, Excel-
  authored files populate the fields, unmodeled attrs ride dirty
  writes, no gratuitous element when no defaults are set.

Refs #426

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(cli): missing -o is a named usage error; lint accepts positional file

Two arg-shape ergonomics fixes from field use:

1. Write commands invoked without -o reported 'Error: Internal:
   output required' — a usage error masquerading as an internal one.
   requireOutput now takes the CLI verb and every one of the 40
   dispatch sites passes its own ('recalc requires -o <out.xlsx>
   (or -i to modify in place)'); requireOutputAction shares the
   same message.

2. 'xl lint <file>' — the form every fresh user tries first — was
   rejected with 'Unexpected argument'. lint takes exactly one file
   and writes nothing, so it now accepts the positional form
   alongside -f: giving the file both ways or not at all exits 2
   with a hint naming both forms.

Refs #422

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ooxml): prune parts orphaned by sheet removal from the write

Deleting a sheet used to leave its whole dependent chain — drawing,
chart, chart colors/style, exclusive media, their .rels and their
[Content_Types].xml registrations — riding the preserved-part copy
loop forever. On a write after a removal, parts reachable ONLY via a
removed sheet's relationship closure now fall out of both the copy
loop and the content types.

Bounded on both sides: anything reachable from ANY surviving part
survives (a shared image stays — the survivor walk runs over source
rels, which same-path drawing regeneration keeps verbatim), and only
the removed sheets' closure is candidate (pre-existing orphans ride
through untouched — no general package GC).

Refs #417

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(display): NumFmtFormatter built-in arms render through FormatCodeParser

Every built-in enum arm hand-rolled its rendering and drifted from what
the same ECMA-376 format code produces through FormatCodeParser (the
file-declared path, kept verbatim since GH-404): PercentDecimal showed
15.6% where '0.00%' is 15.60%, Decimal detoured through Double, the
serial-typed DateTime arm used Java's 12-hour 'h' pattern, Text leaked
BigDecimal.toString, out-of-range date serials fabricated 1899 dates
instead of Excel's ###### fill.

Rewrite the arms to render through a pre-parsed cache of
FormatCodeParser(NumFmt.formatCode(fmt)), making the programmatic enum
and a file-declared equal code identical by construction; two property
tests pin that identity for every built-in variant over numbers and
DateTimes. The calendar variants render straight off the LocalDateTime
(no lossy serial round-trip).

Two FormatCodeParser gaps blocked the unification and are fixed here:
- scientific notation: E+/E- exponent tokens ('0.00E+00' id 11,
  '##0.0E+0' id 48 engineering) previously rendered as garbage literals
- hour clock: 'h' always rendered 12-hour; ECMA-376 SS18.8.31 says
  12-hour only when the section carries AM/PM, else 24-hour

Deliberate re-pins, each against its format code: PercentDecimal
0.156 -> 15.60% ('0.00%' forces two decimals); h:mm:ss of 14:30:45 ->
14:30:45 (no AM/PM => 24-hour); inferred-percent interpolator pin
50.0% -> 50.00% (PercentDecimal is '0.00%').

Refs #410

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs: wave 20 integration — CHANGELOG, lint positional form, test counts

Refs #408 #410 #416 #417 #420 #422 #426 #433 #434

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cli: positional put bypasses smart detection — ISO dates land as text; format:"date" on non-ISO yields Text wearing a Date numFmt

1 participant