feat(merge-yaml): support sequence index in paths#110
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. ✨ 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 sequence indexing in YAML merging, allowing paths like name[idx] to address specific elements within a list. The implementation includes updates to path parsing, value retrieval, and assignment logic to handle these indexed segments, supported by a comprehensive suite of new unit tests. A review comment identifies a potential issue where collect_dotted_paths might generate unparseable paths for root-level sequences and provides a code suggestion to skip processing sequences when the prefix is empty.
| Value::Sequence(seq) => { | ||
| for (idx, elem) in seq.iter().enumerate() { | ||
| let path = format!("{prefix}[{idx}]"); | ||
| out.push(path.clone()); | ||
| collect_dotted_paths(elem, &path, out); | ||
| } | ||
| } |
There was a problem hiding this comment.
The collect_dotted_paths function now attempts to process sequences at the root level (when prefix is empty). However, the path syntax used by kata (and specifically parse_segments in merge_path.rs) does not support root-level indices like [0]. If a YAML document has a sequence at the root and a regex path is used, this will generate unparseable paths, leading to errors during the merge process. Since value_at_path and set_at_path also require the root to be a mapping, it is safer to skip sequences at the root, maintaining the behavior of the previous implementation.
Value::Sequence(seq) => {
if prefix.is_empty() {
return;
}
for (idx, elem) in seq.iter().enumerate() {
let path = format!("{prefix}[{idx}]");
out.push(path.clone());
collect_dotted_paths(elem, &path, out);
}
}There was a problem hiding this comment.
@gemini-code-assist Right, that was a real bug — parse_segments rejects the bare [N] form, and value_at_path / set_at_path already require the root to be a mapping. Fixed in 0149c74 by gating the Value::Sequence arm on !prefix.is_empty(), plus a regression test (collect_dotted_paths_skips_root_level_sequence).
There was a problem hiding this comment.
Thank you for the update and for adding the regression test. The implementation looks correct and addresses the potential issue with root-level sequences while maintaining consistency with the existing path parsing logic.
Mirrors the `name[idx]` extension shipped for `how = "merge-toml"` in #108 to `how = "merge-yaml"`. So a template that writes `paths = ["servers[0]"]` owns only the first element of a YAML sequence and consumers can freely append more entries without `kata apply` clobbering them. Implementation: - merge_yaml.rs imports the shared `PathSeg::{Key, KeyIndex}` + `parse_segments` from merge_path.rs and walks segments instead of splitting on `.`. - `value_at_path` descends `Mapping → Sequence → Mapping` chains via a borrowed `&Mapping` helper (`value_at_mapping_path`), cloning only the leaf value the caller will assign elsewhere. - `set_at_path` gains an `ensure_seq_element` helper that bootstraps a missing sequence to index 0, pushes into an empty existing sequence, refuses to clobber a non-sequence slot, and silently no-ops on out-of-range indices for non-empty existing sequences (same conservative rule as merge-toml). - `collect_dotted_paths` now descends into `Value::Sequence`, emitting `prefix.key[N]` paths so regex specs like `//^servers\[0\]$//` can target individual elements. Root- level sequences are skipped — `parse_segments` rejects the bare `[N]` form and `value_at_path` / `set_at_path` both require the root to be a mapping, so emitting unparseable paths there would just create errors downstream (Gemini #110 review). Unlike merge-toml's `ArrayOfTables` (table-only elements), YAML sequences are uniform — `tags[0]` works for `tags: [foo, bar]` and replaces the string at index 0 directly. Intermediate `KeyIndex` steps still expect the existing element to be a Mapping (or absent, in which case the bootstrap inserts an empty mapping) so the next `Key` step can descend. Tests: 10 new tests under `modes::merge_yaml` mirroring merge- toml`s #107 coverage — bootstrap, index-0 replacement, out-of- range skip, refuse-to-clobber, field-inside-element, scalar sequence element, idempotency, regex-targets-element, `collect_dotted_paths` white-box, and the Gemini-review regression for root-level sequences.
yukimemi
force-pushed
the
feat/merge-yaml-array-index
branch
from
a4bafb8 to
0149c74
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
name[idx]path syntax tohow = "merge-yaml"so a template can own a single sequence element while leaving the rest under consumer control.value_at_path/set_at_path/collect_dotted_pathsnow consume the sharedPathSegfrommerge_path.rsinstead of splitting on..ensure_seq_elementhelper bootstraps a missing sequence to index 0, pushes into an empty existing sequence, refuses to clobber non-sequence slots, and silently no-ops on out-of-range indices for non-empty sequences (same conservative rule merge-toml uses).Motivating example
Notes
ArrayOfTables(table-only elements), YAML sequences are uniform —tags[0]works for atags: [foo, bar]list of scalars and replaces the string at index 0 directly.KeyIndexsteps still expect the element at that index to be a Mapping (or absent, in which case the bootstrap inserts an empty mapping) so a followingKeystep can descend.Test plan
cargo test --lib modes::merge_yaml— 17 tests pass (9 new: bootstrap, idx-0 replacement, out-of-range skip, refuse-to-clobber non-sequence, field-inside-element, scalar sequence element, idempotency, regex-targets-element,collect_dotted_pathswhite-box).cargo fmt --all -- --checkcargo make clippycargo make lock-check🤖 Generated with Claude Code