Repository navigation
Keep emptied [table]-header tables (incl. out-of-order fragments) in the output - #688
mayur-dhavan wants to merge 18 commits into
Conversation
Tables left with no items after removals now emit their header again instead of vanishing from dumps() (python-poetry#553).
Covers dotted-key and explicit tables emptied by removals, and updates the out-of-order deletion expectation (python-poetry#553).
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Prior-art note for reviewers: #590 (open since Aug 26) fixes the same dotted-key super-table half of #553 with a similar approach, and explicitly leaves out-of-order tables untouched. This PR covers that case plus the remaining out-of-order fragment path (an emptied fragment no longer renders when a sibling fragment holds the table's items, and an emptied out-of-order table keeps its |
Tables materialized from dotted keys stay as they were (covered by python-poetry#590); emptied tables from [table] headers keep their header.
for more information, see https://pre-commit.ci
Drop the dotted-key regression test (covered by python-poetry#590); keep out-of-order and explicit-table cases plus explicit deletion checks.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Update: as promised above, this PR has now been narrowed to only the non-dotted-key / out-of-order-fragment half, removing the dotted-key change that overlapped #590. Title and body rewritten to the precise residual scope; the dotted-key case renders exactly as on master. Full suite: 1206 passed. |
|
The sibling-fragment check looked at the root body, so an unrelated same-named top-level table could suppress the header of an emptied nested table.
Add test for emptied nested table with same named top-level table.
|
Good catch - that case exposed a real bug in the first version of this change. The emptied Fixed in the latest commit: the check now compares only within the container body that actually owns the fragment. Your example now renders: which round-trips to exactly the in-memory document ( |
for more information, see https://pre-commit.ci
|
You need to put more effort into critical review of your fixes |
Emptied tables of an out-of-order (fragmented) parent were serialized without checking the other fragments, producing invalid TOML: an emptied fragment could emit its [header] while another fragment still rendered the same table, defining it twice. The dedup now considers the root fragment group (tuple entries in the root container map): an emptied fragment only keeps its header when no fragment of the group renders real content, and a shared per-render set guarantees each emptied header is emitted at most once. Super tables can also keep their header when they would otherwise vanish while still being present in the document.
for more information, see https://pre-commit.ci
Emptied tables of an out-of-order (fragmented) parent were serialized without checking the other fragments, producing invalid TOML: an emptied fragment could emit its [header] while another fragment still rendered the same table, defining it twice. The dedup now considers the root fragment group (tuple entries in the root container map): an emptied fragment only keeps its header when no fragment of the group renders real content, and a shared per-render set guarantees each emptied header is emitted at most once. Super tables also keep their header when they would otherwise vanish while still being present in the document.
for more information, see https://pre-commit.ci
Cover the cases where a deleted table's parent has later or interleaved fragments of the same root table (the emptied fragment must be dropped so the output never redefines a table), the case where every fragment is emptied (each emptied sub-table keeps exactly one header), and deletion of an implicit table's last child (the emptied parent is kept).
for more information, see https://pre-commit.ci
|
You're right, and thank you for the probes. Both reproduce on the previous commit and both outputs were invalid TOML (duplicate / conflicting table headers). The root cause: my dedup only compared a table against its same-container siblings, so it completely missed the case that matters here - fragments of the same out-of-order root table live in separate container bodies, keyed as a tuple in the root container's map. Reworked the logic: when an emptied table would emit its Your cases now:
Both outputs parse back with |
|
Closing this. Independent validation of the latest head (property-based fuzzing over nesting / ordering / deletion combinations) found that the emptied-table handling still produces invalid TOML for some out-of-order fragment shapes - for example Thank you @dimbleby for the time and the sharp probes - they exposed the first round of these problems and made the real difficulty of this corner of the serializer clear. |
Fixes the remaining half of #553 (see "Scope" below - #590 covers the dotted-key half).
Problem
After removing the last key of a table that was materialized from
[table]headers (not from a dotted key),dumps()silently drops the table even though it is still part of the document:The same happens to emptied fragments of an out-of-order table:
del doc["bar"]["a"]leavesbar == {}in the document, butdumps()renders nothing for it. The emptied table's body only holds aNullplaceholder, sois_super_table()classifies it as a super table and no header is emitted. Empty tables are valid TOML ("Empty tables are allowed and simply have no key/value pairs within them", TOML v1.0.0), and dropping them loses data on round-trip.Fix
In
Container._render_table, when a table with a non-dotted key holds no real items (only whitespace orNullplaceholders), emit its header anyway. For fragments of an out-of-order table, a fragment is skipped when a sibling fragment still holds the table's items, so no duplicate headers appear.Deleting a table itself (
del doc["x"]) still removes it from the output entirely; only tables that remain in the document keep their header. This extends the direction of the merged #567 (emptied[[aot]]renders asa = []) from arrays of tables to plain tables.Scope
#590 fixes the dotted-key half of #553 (emptied tables materialized from
x.y.z = 1style keys,key.is_dotted()), and explicitly leaves out-of-order tables and non-dotted keys untouched. This PR covers exactly that remainder: non-dotted keys, including out-of-order fragments. The two PRs are complementary; neither changes the other's behavior. Happy to rebase if #590 lands first.Behavior change
test_delete_out_of_order_table_keypreviously asserted that an emptied out-of-order table vanishes from the output. Sincedel doc["bar"]["a"]leavesbar == {}in the document, dropping it loses data; the test now expects the empty[bar]header. Round-trip checked with stdlibtomllib: the new output parses back to exactly the in-memory document.Testing
test_delete_out_of_order_table_key(updated expectation) and newtest_deleting_last_child_table_keeps_emptied_parentfail on master, pass with this fix.test_emptied_explicit_table_still_serializes(explicit[x.y]header survives emptying) andtest_deleting_table_removes_it_from_output(whole-table deletion still removes it).This PR was prepared with AI assistance (an automated coding agent acting on behalf of the author).