feat: add reordering option - #260
Merged
Merged
Conversation
Reorders root key-values, [table]/[[array]] section blocks, and table-body rows to match the key order of the JS object passed to patch(), carrying each entry's owned comments along with it. Off by default; existing behavior is unchanged unless the caller opts in. - src/toml-format.ts: updateOrder?: boolean, default false, wired through validation, resolveTomlFormat, the constructor, default(), and forced to false in auto-detection (the existing document's order says nothing about intent). - src/diff.ts: DiffOptions/Move.key, and a new order-emission step in compareObjects (simulate-and-splice, mirroring how compareArrays already emits Move for arrays). A no-options diff() call still emits zero Moves for a pure permutation -- the existing API-compat guarantee. - src/update-order.ts (new): applyKeyOrderMoves. Groups a container's slots into movable/pinned/fixed-anchor units -- this is what makes an [[array-of-tables]] or a [table]+[table.sub] move as one rigid block, and what makes a non-contiguous dotted-key group (or the TOML spec's own "valid but discouraged" out-of-order table example) bail out safely instead of corrupting anything -- computes the target order per validity partition (root key-values can never follow a section), and relays out via shiftNode. Never calls insert()/remove(), which would re-dirty offsets nothing downstream flushes. - src/patch.ts: threads updateOrder into the diff call, snapshots prePatchNodes before any change is applied (so a key Added by this same patch can't adopt a preceding comment run), routes object-key Moves into a separate bucket, and runs the reorder pass last, after every other structural change. See docs/PLAN-Update-Order.md for the full design.
… value into a section parseJS -> formatTopLevel unconditionally hoists any inline-table/AOT- shaped root key into its own [section]/[[array]] block via remove-then-APPEND (a genuine TOML requirement when the value becomes an actual section: root scalars must precede section headers) -- but it applies this to ANY nested object at the default inlineTableStart, even when the existing document represents it as a plain inline value with no such constraint. That silently reordered such a key after every scalar in updated_js, before the diff ever ran, independent of updateOrder and regardless of the caller's request. Fixes it by re-deriving the root key order from a throwaway, un-hoisted parse (inlineTableStart: 0 disables the section conversion) and re-keying updated_js's top level to match, only when updateOrder is set. Only the top-level key order is affected -- values, nested levels, and the actual output format of new content are untouched.
Rounds out the remaining plan.md §5 test items not yet covered by
update-order.test.ts's own behavior matrix:
- diff.test.ts: no-options guarantee (zero Moves for a pure
permutation), the exact Move shape ({type,path,from,to,key}), array
Moves staying bare-ordinal/keyless, identity permutation, and
composing with Add.
- toml-format.test.ts: default false, never auto-detected,
validateFormatObject accept/reject, the constructor's positional-
argument wiring guard, and a console.warn regression guard (a plain
patch(x, y) call must never warn about the option).
- validate-cst.test.ts: threaded an optional format param through
getOverlaps/getInverted/expectConsistent (existing call sites
unaffected by the default), added the two updateOrder-specific
structural invariants (ascending member order; no overlapping
sibling members, both scoped to exclude comments so a normal
same-line trailing comment isn't a false positive), and ran the
full update-order.test.ts behavior matrix through them.
Move.from/to are indices into the FULL member key sequence compareObjects saw (every top-level key of a container's JS object, movable or not) -- but applyContainerMoves was replaying only the "relevant" (movable) moves against a sequence that had ALREADY excluded fixed-anchor keys (non-contiguous groups like the TOML spec's own "valid but discouraged" [fruit.apple]/[animal]/[fruit.orange] example). That silently misinterpreted the indices: an in-range move for a genuinely movable sibling could look like a no-op purely because the index space had shifted once the fixed anchor's slot was dropped from it. Fixes it by replaying over the full sequence, with fixed anchors included as unique NUL-prefixed placeholder tokens (never themselves targeted -- TOML keys can never contain a control character, so this can't collide with a real key), and filtering only the RESULT down to movable keys afterward. Adds a regression test: [zebra] and [animal] now correctly swap around a fixed [fruit.apple]/[fruit.orange] anchor, which stays exactly where it is, both in the reordered output and in the structural invariant checks (validate-cst.test.ts).
There was a problem hiding this comment.
Pull request overview
Adds an opt-in formatting option (updateOrder) that allows patch() to reorder TOML output to match the key order of the input JS object, while preserving comment ownership and maintaining TOML validity constraints.
Changes:
- Introduces
TomlFormat.updateOrder(defaultfalse) and threads it through validation, resolution, and auto-detect behavior. - Extends
diff()to optionally emit object-keyMovechanges (with{ key }) and applies those moves at the end of patching via a new reorder pass. - Adds a comprehensive test matrix for updateOrder behavior plus additional CST consistency invariants.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/update-order.ts | Implements the updateOrder reorder pass over CST containers, including comment ownership and safety guards. |
| src/toml-format.ts | Adds the updateOrder option (defaults, validation, resolve wiring, and documentation). |
| src/patch.ts | Wires updateOrder through diffing and applies reorder moves at the end of patching. |
| src/diff.ts | Adds DiffOptions.updateOrder and emits object-key Move changes when enabled. |
| src/tests/validate-cst.test.ts | Adds updateOrder-specific structural invariants and runs scenarios through them. |
| src/tests/update-order.test.ts | Adds behavior-matrix tests for updateOrder on/off, including comments and edge cases. |
| src/tests/toml-format.test.ts | Tests updateOrder defaults, validation, and resolveTomlFormat wiring. |
| src/tests/patch.test.ts | Updates/extends root-key placement tests to cover updateOrder behavior. |
| src/tests/diff.test.ts | Tests diff() compatibility and the new updateOrder Move emission shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A gap in the existing coverage: the inline-table-value reorder test had no [section] present at all, and the inlineTableStart: 0 validity- partition test only used a bare scalar. Neither confirmed that an inline-table-VALUED root key (a plain KeyValue, not a Table -- so classified as root-KV, not section, for partition purposes) reorders correctly relative to other root keys while a real section stays fixed after both, or that the partition still holds when the key being asked to jump past a section happens to be an inline table rather than a scalar. Also drops an unused `parse` import from update-order.test.ts flagged by the editor.
Three situations where a repositioning request is silently left
unhonored ("did nothing" is the safe failure mode, but it was
invisible to the caller):
- A move targets a non-contiguous group (a dotted-key group or the
TOML spec's own "valid but discouraged" out-of-order table
pattern) -- the entry stays exactly where it is.
- A move targets a location updateOrder doesn't resolve at all yet:
a dotted-key implicit table, or an interior of an inline table/
array-of-tables entry.
- The requested order itself asks for a root key-value to land after
a section header, which TOML cannot represent -- each partition
(root key-values, sections) keeps its own requested relative order
instead of the literal interleaving asked for.
All three are consolidated into a single console.warn per patch()
call (matching validateFormatObject's existing convention), naming
the full dotted path of each affected entry. Only fires when
updateOrder actually attempted something and couldn't fully honor
it -- never for a successful reorder, and never when the option is
off.
Flagged by a GitHub Copilot review comment. It was never read inside the function body -- every use of `document` in this file is in its caller, applyKeyOrderMoves.
A table->scalar edit regenerates a fresh KV/Table node via replace(), in place of the entry it replaces. isEligibleForLeading (update-order.ts) keyed eligibility off prePatchNodes identity alone, so that fresh node -- despite representing the same entry, not a new one -- was wrongly treated the same as a genuinely Added key, and a leading comment above it got left pinned instead of travelling with it during an updateOrder reorder in the same patch. Fixed by adding each replacement node into the same set at its two replace() call sites in the isTable(existing) branch, and renaming it from prePatchNodes to commentEligibleNodes to reflect what it now tracks. Flagged by a GitHub Copilot review comment on PR #260.
Root-causes the fix in ce612c0, and documents two adjacent issues found while investigating it: handleStructuralEdit's freshKV can still orphan a leading comment (same class as the known writer.remove() gap in PLAN-Update-Order.md), and replaceEmptiedTableArrays can place an emptied-AOT key after an existing table section, producing invalid placement independent of updateOrder. Neither is fixed here.
A key-value that physically follows a [table] header parses as a member of
that section. Both handleStructuralEdit and replaceEmptiedTableArrays
appended their regenerated root key at the end of the document, so whenever
any section existed the key was silently reparented -- data corruption, not
just placement:
[[i]] + [other], patched {i: 42, other: {b: 2}}
reparsed as {other: {b: 2, i: 42}}
The isTable(existing) branch already guarded against this with logic inlined
after its replace(). Extracted that into hoistRootKeyValueAboveTables() /
rootKeyValueInsertIndex() and applied it to all three sites.
handleStructuralEdit is also restructured to swap the fresh key-value in via
replace() rather than remove-then-append, mirroring the isTable path. In
place, the key keeps its blank-line budget (appending doubled it, and
append-then-move crashed to-toml.ts on stale loc bookkeeping) and stays
adjacent to its leading comment -- which resolves the comment-orphaning
symptom without needing the separate writer.remove() work. It now also
registers its freshKV as comment-eligible, since it is a replacement.
Existing tests missed all of this by only exercising documents with no
competing section.
Adds the option to the TomlFormat class signature and the Formatting Options list, with worked examples for the default-off behaviour, comments travelling with their entry, and the root-key/section validity partition. Notes the shapes that are left in place with a console.warn. Also cross-references the option from the Comment Ownership section, and adds the changelog entries for the feature and for the root-key placement fix (which affected released behaviour, independent of updateOrder). All README examples verified against actual patch() output.
DecimalTurn
force-pushed
the
dev-update-order
branch
from
July 28, 2026 00:03
c3dd6fa to
12f4e24
Compare
# Conflicts: # src/patch.ts
DecimalTurn
marked this pull request as ready for review
July 28, 2026 01:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #174