feat(merge-toml): support inline array indexing in paths#112
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR extends TOML merge functionality to support inline-array element access using ChangesTOML inline-array indexing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for indexing and merging inline arrays in TOML files, enabling path-based access and updates to specific array elements (e.g., name[idx]). Key changes include updating path collection to emit element-level paths, modifying the path traversal and setter logic to handle inline arrays alongside existing Array of Tables (AoT) support, and adding a suite of unit tests. Feedback focuses on optimizing the ensure_array_element helper function by utilizing the entry API to eliminate redundant lookups.
| if !table.contains_key(key) { | ||
| if idx != 0 { | ||
| return None; | ||
| } | ||
| let mut arr = Array::new(); | ||
| arr.push(Value::from(0i64)); | ||
| table.insert(key, Item::Value(Value::Array(arr))); | ||
| return table.get_mut(key)?.as_array_mut()?.get_mut(0); | ||
| } | ||
| let entry = table.get_mut(key)?; | ||
| let arr = entry.as_array_mut()?; | ||
| if arr.is_empty() { | ||
| if idx != 0 { | ||
| return None; | ||
| } | ||
| arr.push(Value::from(0i64)); | ||
| return arr.get_mut(0); | ||
| } | ||
| if idx >= arr.len() { | ||
| return None; | ||
| } | ||
| arr.get_mut(idx) |
There was a problem hiding this comment.
The ensure_array_element function performs multiple redundant lookups on the table (e.g., contains_key, insert, and get_mut). This can be optimized and made more idiomatic by using the entry API, which performs a single lookup for both checking existence and retrieving or inserting the entry.
match table.entry(key) {
toml_edit::Entry::Vacant(v) => {
if idx != 0 {
return None;
}
let mut arr = Array::new();
arr.push(Value::from(0i64));
v.insert(Item::Value(Value::Array(arr)))
.as_value_mut()?
.as_array_mut()?
.get_mut(0)
}
toml_edit::Entry::Occupied(mut o) => {
let arr = o.get_mut().as_array_mut()?;
if arr.is_empty() {
if idx != 0 {
return None;
}
arr.push(Value::from(0i64));
return arr.get_mut(0);
}
if idx >= arr.len() {
return None;
}
arr.get_mut(idx)
}
}There was a problem hiding this comment.
@gemini-code-assist Done in 2b70101. Switched ensure_array_element to the toml_edit::Entry API and applied the same refactor to ensure_aot_element for symmetry — both helpers now do a single lookup per call. One wrinkle worth noting for next time: OccupiedEntry::get_mut borrows from the entry, so the resulting reference can't escape the match arm (returning it triggers E0515). slot.into_mut() consumes the entry and yields &'a mut Item with the table's lifetime, which is what the helper needs.
There was a problem hiding this comment.
That's a great catch regarding the OccupiedEntry::get_mut lifetime constraints. Using into_mut() is indeed the correct, idiomatic way to consume the entry and obtain a mutable reference with the appropriate lifetime for the table. Your refactor looks solid and consistent across both helpers.
Closes #111. Extends the `name[idx]` syntax shipped in #108 to inline arrays (`Item::Value(Value::Array(_))`) in addition to the `ArrayOfTables` shape that issue covered. So a template with `paths = ["tasks.on-add.dependencies[0]"]` can own one element of a cargo-make-style `dependencies = ["fmt", "clippy"]` list while the consumer freely appends their own steps at idx 1+. Implementation: - `item_at_table_path` adds an `Item::Value(Value::Array(_))` branch alongside the existing `Item::ArrayOfTables` one. Tries AoT first (the kata#107 shape) and falls through to inline array. Inline-array leaves return `Item::Value(elem.clone())`; a continuing path after an inline-array index bails out (the elements are scalars / inline values that don't carry sub- paths). - `set_in_table` at a leaf `KeyIndex` now dispatches on the incoming `Item` variant — `Item::Table` → `ensure_aot_element`, `Item::Value` → new `ensure_array_element`. A shape mismatch (existing is AoT but incoming is inline, or vice versa) naturally no-ops inside the helper because each one checks the existing slot's actual type. - New `ensure_array_element` helper mirrors `ensure_aot_element`: bootstrap only at `idx == 0` (missing entry or empty array), no-op on out-of-range and on non-array existing slots. Both helpers use the `toml_edit::Entry` API (single lookup per call) instead of the older `contains_key`/`get_mut` split — Gemini #112 review. - `collect_dotted_paths` emits `prefix[N]` for inline-array entries so regex specs like `//^tags\[0\]$//` can target them. Inline-array elements aren't recursed into (their values are scalars / inline tables; `parse_segments` doesn't accept `[N][M]`). Tests: 9 new `modes::merge_toml::tests` mirroring the #107 coverage for inline arrays — bootstrap, idx-0 replacement, out-of-range skip, refuse-to-clobber non-array, scalar element replacement, idempotency, shape mismatch (AoT existing / inline incoming) refuse, regex-targets-element, and `collect_dotted_paths` white-box. Total `merge_toml` tests: 27 passing.
yukimemi
force-pushed
the
feat/merge-toml-inline-array-index
branch
from
451c040 to
2b70101
Compare
1 participant
Summary
Closes #111. Extends the
name[idx]syntax shipped in #108 to inline arrays (Item::Value(Value::Array(_))) in addition toArrayOfTables. So a template withpaths = ["tasks.on-add.dependencies[0]"]can own a single element of a cargo-make-styledependencies = ["fmt", "clippy"]list while the consumer freely appends more at idx 1+.Motivating example
Implementation
item_at_table_pathadds anItem::Value(Value::Array(_))branch alongsideItem::ArrayOfTables. Tries AoT first (the kata#107 shape), falls through to inline array. Inline-array leaves returnItem::Value(elem.clone()); a continuing path after an inline-array index bails out (the elements are scalars / inline values that don't carry sub-paths).set_in_tableat a leafKeyIndexnow dispatches on the incomingItemvariant —Item::Table→ensure_aot_element,Item::Value→ newensure_array_element. A shape mismatch (existing is AoT but incoming is inline, or vice versa) naturally no-ops because each helper checks the existing slot's actual type.ensure_array_elementhelper mirrorsensure_aot_element's rules:idx != 0→ no-opidx == 0→ bootstrap withValue::Array(vec![placeholder]), return slot 0idx == 0→ push placeholder, return slot 0idx < len→ return slotidxcollect_dotted_pathsemitsprefix[N]for inline-array entries so regex specs like//^tags\[0\]$//can target them. Inline-array elements aren't recursed into —parse_segmentsdoesn't accept[N][M]and there's noKeystep into a scalar.Scope notes
Itemtype. Existing PRs and templates keep working.Item::ArrayOfTables/Item::Value(Value::Array(_))are the two array shapes supported. Inline tables and value arrays containing nested arrays still terminate the walk (path syntax doesn't have a way to chain[N][M]).Test plan
cargo test --lib modes::merge_toml— 27 tests pass (9 new: idx-0 replacement, bootstrap, out-of-range skip, refuse-to-clobber non-array, scalar element replacement, idempotency, shape-mismatch AoT-vs-inline refuse, regex-targets-element,collect_dotted_pathswhite-box).cargo fmt --all -- --checkcargo make clippycargo make lock-check🤖 Generated with Claude Code
Summary by CodeRabbit
dependencies[0]).