Skip to content

Keep unwrap() key order after a value is replaced - #591

Open
youdie006 wants to merge 2 commits into
python-poetry:masterfrom
youdie006:fix/unwrap-key-order
Open

Keep unwrap() key order after a value is replaced#591
youdie006 wants to merge 2 commits into
python-poetry:masterfrom
youdie006:fix/unwrap-key-order

Conversation

@youdie006

Copy link
Copy Markdown

Summary

unwrap() returns keys in the wrong order after a value is replaced.

doc = parse("a = 1\nb = 2\nc = 3\n")
doc["b"] = 9

dumps(doc)          # 'a = 1\nb = 9\nc = 3\n'
list(doc.keys())    # ['a', 'b', 'c']
list(doc.unwrap())  # ['a', 'c', 'b']   <-- 'b' moved to the end

_replace_at deletes the key from _map and re-inserts it (container.py:916-917), which moves it to the end of the dict's insertion order, while _body[idx] correctly keeps the original slot. dumps() and keys() walk _body and stay right; unwrap() walks _map and does not. It reproduces for every scalar and array replacement value.

This is against the README's own promise:

a parser that preserves all comments, indentations, whitespace and internal element ordering
README.md:18

and against the assumption unwrap() was written on when it moved to _map in #521:

_map iterates in the same insertion order as the old self.items().
container.py:70-71

That sentence is true until the first replacement, which is why it held when #521 landed.

The change

Take the order from the body index rather than from _map insertion order. The _map fast path that #521 added is kept — the point of that change was avoiding __getitem__ rebuilding a SingleKey per key, and that still holds — and dumps() becomes the reference for order rather than a second, divergent source.

I looked at the other _map.items() loops before choosing this over rewriting _replace_at: container.py:542 and :583 only adjust index values and do not care about order, so unwrap() is the sole order-sensitive consumer.

Not changed: replacements that genuinely move an element

A bare key promoted to a table has to be emitted after the inline entries, and dumps() moves it:

doc = parse("a = 1\nb = 2\n")
doc["a"] = {"x": 1}
dumps(doc)          # 'b = 2\n\n[a]\nx = 1\n'
list(doc.unwrap())  # ['b', 'a']  -- correct, and unchanged by this PR

Ordering by the body index keeps this case as it was, since the body really did move. There is a test pinning it so the distinction cannot regress.

Tests

Two added to tests/test_toml_document.py: one asserting the order is preserved on a scalar replacement, one pinning the table-promotion case above.

Verified by reverting only tomlkit/container.py and keeping the tests — the first fails with assert ['a', 'c', 'b'] == ['a', 'b', 'c'], the second still passes, so the new coverage separates the bug from the behaviour that should not change.

Full suite: 1052 passing before, 1054 after.

Agent Drafting Metadata

  • Agent: Claude Code
  • Model: Claude Opus 5
  • Notes: The reproduction, the red/green check and the suite runs above were executed against this branch. I reviewed and verified the change before opening the PR; happy to adjust anything on request.

youdie006 and others added 2 commits August 27, 2026 11:12
_replace_at() removes the key from _map and re-inserts it, which moves it
to the end of the dict's insertion order while _body keeps the original
slot. dumps() and keys() walk _body and stay correct; unwrap() walks _map
and does not:

    doc = parse("a = 1\nb = 2\nc = 3\n")
    doc["b"] = 9
    dumps(doc)          # 'a = 1\nb = 9\nc = 3\n'
    list(doc.keys())    # ['a', 'b', 'c']
    list(doc.unwrap())  # ['a', 'c', 'b']

unwrap() moved to _map in python-poetry#521 on the stated assumption that it "iterates
in the same insertion order as the old self.items()", which a replacement
breaks. Order comes from the body index instead, so the _map fast path is
kept and dumps() stays the reference.

Replacements that genuinely move an element still move: a bare key
promoted to a table has to be emitted after the inline entries, and
unwrap() follows the body there too. Both cases are covered.
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.

1 participant