You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#107 / #108 extended paths syntax with name[idx] for TOML ArrayOfTables (the [[name]] form). merge_toml::item_at_path
guards with as_array_of_tables()?, so a KeyIndex step against
an inline array of values — dependencies = ["a", "b", "c"],
an Item::Value(Value::Array(_)) — falls through to None and
the path is a silent no-op.
This came up while auditing the original issue's motivating case:
pj-rust lists ... and tasks.on-add in Makefile.toml's
[merge-toml paths]. Both manage arrays / dependencies arrays
wholesale. (#107)
In cargo-make, tasks.on-add is conventionally tasks.on-add.dependencies = ["fmt-check", "clippy", "test", ...]
— that's an inline string array. With today's #108 impl, a
template that wrote paths = ["tasks.on-add.dependencies[0]"]
would silently no-op (the path walker bails at the [0] step),
leaving the consumer no per-element addressing for that case.
#107's Proposed section specifies "when the literal path
resolves to an ArrayOfTables", so the spec itself is scoped to
AoT — this issue isn't a regression, it's a gap the spec didn't
cover. Kanade's specific workflow (multiple [[hooks.post_create]])
is already served by #108's AoT support; the tasks.on-add
example in #107 is the part that motivates this follow-up.
Proposed
Extend the merge-toml walker so a PathSeg::KeyIndex(k, i) step
also accepts an Item::Value(Value::Array(_)) parent:
[[file]]
src = "Makefile.toml"how = "merge-toml"paths = [
"tasks.on-add.dependencies[0]", # kata owns the first dep# consumer can append more
]
Semantics — mirror the AoT rules from #108 so the surface stays
uniform:
Read (item_at_path): if the entry at k is an inline Value::Array, return Item::Value(array.get(i).cloned()) when
in range; None otherwise.
Write (set_at_path):
entry missing AND i == 0 → bootstrap with Value::Array(vec![incoming]) (mirrors AoT bootstrap)
entry present as Value::Array, i < len → replace element i in place
entry present as Value::Array, empty AND i == 0 → push
any other out-of-range / wrong-shape → no-op (refuse-to-
clobber, same as AoT)
Type check at the leaf: Value::Array only holds Values,
not nested Items. If the incoming value is an Item::Table
(the AoT-style leaf) the cast fails — refuse rather than
silently coerce.
Implementation notes
merge_toml::item_at_table_path already takes the &Table
borrow; add an Item::Value(Value::Array(_)) branch alongside
the existing Item::ArrayOfTables one.
merge_toml::ensure_aot_element is AoT-specific; a parallel ensure_array_element (returning &mut Value) keeps the two
shapes cleanly separated.
collect_dotted_paths already emits key[N] for the AoT case;
extend it to also iterate Item::Value(Value::Array(_)) and
emit per-element paths so regex specs can target inline-array
elements too.
merge-yaml is already fine here — YAML sequences are uniform
and feat(merge-yaml): support sequence index in paths #110 handles scalar elements (the merge_can_replace_scalar_sequence_element test covers it).
So this issue is TOML-only.
Tests to add
Mirror merge-toml's #107 coverage but for Value::Array:
replace element 0 of an inline string array, leave the rest
bootstrap a missing inline array at idx 0
skip out-of-range index on a non-empty existing array
refuse to clobber non-array slot at the index path
idempotency under re-apply
regex form //^tasks\.on-add\.dependencies\[0\]$// targets one
element
Workaround for now
For consumers that need to extend an inline array, fall back to merge-section (custom managed-block) on the file, or restructure
the upstream template so the array becomes an ArrayOfTables
where appropriate (e.g. [[hooks.post_create]] instead of hooks.post_create = [...]). Neither is as clean as the indexed
path addressing #108 already provides for AoT.
Problem
#107 / #108 extended
pathssyntax withname[idx]for TOMLArrayOfTables(the[[name]]form).merge_toml::item_at_pathguards with
as_array_of_tables()?, so aKeyIndexstep againstan inline array of values —
dependencies = ["a", "b", "c"],an
Item::Value(Value::Array(_))— falls through toNoneandthe path is a silent no-op.
This came up while auditing the original issue's motivating case:
In cargo-make,
tasks.on-addis conventionallytasks.on-add.dependencies = ["fmt-check", "clippy", "test", ...]— that's an inline string array. With today's #108 impl, a
template that wrote
paths = ["tasks.on-add.dependencies[0]"]would silently no-op (the path walker bails at the
[0]step),leaving the consumer no per-element addressing for that case.
Scope vs #107
#107's Proposed section specifies "when the literal path
resolves to an
ArrayOfTables", so the spec itself is scoped toAoT — this issue isn't a regression, it's a gap the spec didn't
cover. Kanade's specific workflow (multiple
[[hooks.post_create]])is already served by #108's AoT support; the
tasks.on-addexample in #107 is the part that motivates this follow-up.
Proposed
Extend the merge-toml walker so a
PathSeg::KeyIndex(k, i)stepalso accepts an
Item::Value(Value::Array(_))parent:Semantics — mirror the AoT rules from #108 so the surface stays
uniform:
item_at_path): if the entry atkis an inlineValue::Array, returnItem::Value(array.get(i).cloned())whenin range;
Noneotherwise.set_at_path):i == 0→ bootstrap withValue::Array(vec![incoming])(mirrors AoT bootstrap)Value::Array,i < len→ replace elementiin placeValue::Array, empty ANDi == 0→ pushclobber, same as AoT)
Value::Arrayonly holdsValues,not nested
Items. If the incoming value is anItem::Table(the AoT-style leaf) the cast fails — refuse rather than
silently coerce.
Implementation notes
merge_toml::item_at_table_pathalready takes the&Tableborrow; add an
Item::Value(Value::Array(_))branch alongsidethe existing
Item::ArrayOfTablesone.merge_toml::ensure_aot_elementis AoT-specific; a parallelensure_array_element(returning&mut Value) keeps the twoshapes cleanly separated.
collect_dotted_pathsalready emitskey[N]for the AoT case;extend it to also iterate
Item::Value(Value::Array(_))andemit per-element paths so regex specs can target inline-array
elements too.
and feat(merge-yaml): support sequence index in paths #110 handles scalar elements (the
merge_can_replace_scalar_sequence_elementtest covers it).So this issue is TOML-only.
Tests to add
Mirror merge-toml's #107 coverage but for
Value::Array://^tasks\.on-add\.dependencies\[0\]$//targets oneelement
Workaround for now
For consumers that need to extend an inline array, fall back to
merge-section(custom managed-block) on the file, or restructurethe upstream template so the array becomes an
ArrayOfTableswhere appropriate (e.g.
[[hooks.post_create]]instead ofhooks.post_create = [...]). Neither is as clean as the indexedpath addressing #108 already provides for AoT.
Related
hooks.post_create[0]) #107 (originating issue / AoT motivation)