-
Notifications
You must be signed in to change notification settings - Fork 0
feat: handle flow sequences gracefully in sync, insert, append, extend_list #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
nathanjmcdougall
merged 11 commits into
main
from
31-sync-should-handle-flow-sequences-gracefully
May 20, 2026
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
d36ad57
feat: add NodeTypeError(PatchError, TypeError) to error hierarchy
nathanjmcdougall 7f43021
feat: raise NodeTypeError from remove_from_list on non-list
nathanjmcdougall 790db9a
feat: sync() falls back to replace on flow sequences
nathanjmcdougall 3620d8f
feat: insert() falls back to replace on flow sequences
nathanjmcdougall 5ba98fa
feat: append() and extend_list() fall back to replace on flow sequences
nathanjmcdougall b773220
Bump lockfile
nathanjmcdougall d012b17
Add specs
nathanjmcdougall 7a73bbb
refactor: pre-detect flow sequences in sync() to preserve sibling for…
nathanjmcdougall 1093d76
feat: raise NodeTypeError from append/insert/extend_list on non-seque…
nathanjmcdougall 17a07c7
fix: recurse into list elements in _flow_seq_replacements and add syn…
nathanjmcdougall b3c11a2
docs: narrow flow sequence comment claim in spec
nathanjmcdougall File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| # Flow Sequence Fallback | ||
|
|
||
| **Date:** 2026-05-20 | ||
| **Issue:** #31 | ||
|
|
||
| ## Problem | ||
|
|
||
| `insert()`, `append()`, `extend_list()`, and `sync()` raise `PatchError` when the target sequence uses flow syntax (e.g. `repos: [a, b, c]` or `repos: []`). The Rust `apply_insert_at` implementation requires a `BlockSequence` tree-sitter node — flow sequences have different grammar structure and are not supported. | ||
|
|
||
| This forces callers to defensively wrap calls in try/except: | ||
|
|
||
| ```python | ||
| try: | ||
| doc = doc.sync("repos", value=repos_list) | ||
| except yamltrip.PatchError: | ||
| doc = doc.upsert("repos", value=repos_list) | ||
| ``` | ||
|
|
||
| The error is surprising because callers expressed high-level intent ("append to this list", "sync this list") and don't expect to care about YAML syntax details. | ||
|
|
||
| ## Design | ||
|
|
||
| When a list-mutating operation hits a flow sequence, fall back to a get→mutate→replace strategy in Python. This eliminates the `PatchError` for flow sequences at the cost of losing inline formatting (flow → block conversion), which is acceptable because: | ||
|
|
||
| 1. An empty flow sequence (`[]`) has no formatting to preserve. | ||
| 2. A non-empty single-line flow sequence cannot contain comments; multi-line flow sequences could in theory, but are vanishingly rare in practice. | ||
| 3. The alternative is crashing. | ||
|
|
||
| ### Affected Methods | ||
|
|
||
| | Method | Current Behavior | New Behavior | | ||
| |--------|-----------------|--------------| | ||
| | `sync()` | `PatchError` when diff produces `insert_at` on flow seq | Catch error, fall back to full `replace` with computed new value | | ||
| | `insert()` | `PatchError` always on flow seq | Get current list, insert in Python, `replace` full list | | ||
| | `append()` | `PatchError` always on flow seq | Get current list, append in Python, `replace` full list | | ||
| | `extend_list()` | `PatchError` always on flow seq | Get current list, extend in Python, `replace` full list | | ||
|
|
||
| ### Detection Strategy | ||
|
|
||
| Catch `PatchError` from `_apply_patches` and inspect the message for the flow sequence signature (`"expected BlockSequence"`). If matched, execute the fallback. If not, re-raise. | ||
|
|
||
|
nathanjmcdougall marked this conversation as resolved.
|
||
| This is preferable to pre-checking the node type because: | ||
| - It keeps the happy path (block sequences) zero-cost. | ||
| - It avoids duplicating Rust-side type checking in Python. | ||
| - It naturally handles mixed patches where only some operations hit flow sequences. | ||
|
|
||
| ### Implementation: `sync()` | ||
|
|
||
| ```python | ||
| def sync(self, *keys: KeyPart, value: Any) -> Document: | ||
| ... | ||
| patches = _compute_patches(old_value, value, normalized) | ||
| if not patches: | ||
| return self | ||
| try: | ||
| return self._apply_patches(patches) | ||
| except PatchError as e: | ||
| if "expected BlockSequence" not in str(e): | ||
| raise | ||
| # Flow sequence — fall back to replacing the entire value | ||
| route = _make_route(normalized) | ||
| op = _core.Op.replace(value) | ||
| return self._apply_patches([_core.Patch(route=route, operation=op)]) | ||
| ``` | ||
|
|
||
| ### Implementation: `insert()` | ||
|
|
||
| ```python | ||
| def insert(self, *keys: KeyPart, index: int, value: Any) -> Document: | ||
| route = _make_route(keys) | ||
| op = _core.Op.insert_at(index=index, value=value) | ||
| patch = _core.Patch(route=route, operation=op) | ||
| try: | ||
| return self._apply_patches([patch]) | ||
| except PatchError as e: | ||
| if "expected BlockSequence" not in str(e): | ||
| raise | ||
| current = self[keys] | ||
| # Use Python list.insert semantics | ||
| new_list = list(current) | ||
| new_list.insert(index, value) | ||
| replace_op = _core.Op.replace(new_list) | ||
| return self._apply_patches([_core.Patch(route=route, operation=replace_op)]) | ||
| ``` | ||
|
|
||
| ### Implementation: `append()` | ||
|
|
||
| ```python | ||
| def append(self, *keys: KeyPart, value: Any) -> Document: | ||
| route = _make_route(keys) | ||
| op = _core.Op.append(value) | ||
| patch = _core.Patch(route=route, operation=op) | ||
| try: | ||
| return self._apply_patches([patch]) | ||
| except PatchError as e: | ||
| if "expected BlockSequence" not in str(e): | ||
| raise | ||
| current = self[keys] | ||
| new_list = list(current) + [value] | ||
| replace_op = _core.Op.replace(new_list) | ||
| return self._apply_patches([_core.Patch(route=route, operation=replace_op)]) | ||
| ``` | ||
|
|
||
| ### Implementation: `extend_list()` | ||
|
|
||
| ```python | ||
| def extend_list(self, *keys: KeyPart, values: Sequence[Any]) -> Document: | ||
| if not values: | ||
| return self | ||
| route = _make_route(keys) | ||
| patches = [ | ||
| _core.Patch(route=route, operation=_core.Op.append(v)) for v in values | ||
| ] | ||
| try: | ||
| return self._apply_patches(patches) | ||
| except PatchError as e: | ||
| if "expected BlockSequence" not in str(e): | ||
| raise | ||
| current = self[keys] | ||
| new_list = list(current) + list(values) | ||
| replace_op = _core.Op.replace(new_list) | ||
| return self._apply_patches([_core.Patch(route=route, operation=replace_op)]) | ||
| ``` | ||
|
|
||
| ## Behavioral Notes | ||
|
|
||
| - The fallback always produces block sequence output. `repos: []` becomes: | ||
| ```yaml | ||
| repos: | ||
| - item1 | ||
| - item2 | ||
| ``` | ||
| - If the entire value is being replaced (e.g. `sync` from `[a]` to `[b]`), no fallback is needed — `Op.replace` works on any node type. | ||
| - Comment preservation: flow sequences cannot contain comments, so no comments are lost. | ||
| - The fallback does NOT apply to `remove()` or `remove_from_list()` — those already work on flow sequences via index paths. | ||
|
|
||
| ## Edge Cases | ||
|
|
||
| | Scenario | Behavior | | ||
| |----------|----------| | ||
| | `sync("key", value=[])` on flow seq `key: [a, b]` | No fallback needed — `_diff_lists` emits a single `replace([])` | | ||
| | `sync("key", value=[a, b])` on `key: []` (flow) | Fallback: replace with `[a, b]` as block | | ||
| | `insert("key", index=0, value="x")` on `key: [a]` (flow) | Fallback: replace with `["x", "a"]` | | ||
| | `append("key", value="x")` on `key: []` (flow) | Fallback: replace with `["x"]` | | ||
| | Nested flow inside block: `- [a, b]` | Fallback applies to inner flow sequence if targeted directly | | ||
|
|
||
| ## Not In Scope | ||
|
|
||
| - Teaching Rust to natively insert into flow sequences (potential future optimization). | ||
| - Converting block sequences to flow sequences. | ||
| - Handling flow mappings (`{a: 1, b: 2}`) — not affected since `sync` on mappings uses `add`/`replace`/`remove`, not `insert_at`. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| # NodeTypeError for Type Mismatch Operations | ||
|
|
||
| **Date:** 2026-05-20 | ||
| **Issue:** #31 | ||
|
|
||
| ## Problem | ||
|
|
||
| When a caller uses a sequence operation (`append`, `remove_from_list`, etc.) on a node that isn't a sequence, or a mapping operation on a non-mapping, the error raised is a bare `PatchError`. This conflates "the operation is structurally impossible given YAML syntax limitations" with "you called the wrong method for this node type." | ||
|
|
||
| The latter is a caller mistake — semantically a `TypeError` — and should be distinguishable from structural failures. | ||
|
|
||
| ## Design | ||
|
|
||
| Introduce `NodeTypeError` as a subclass of both `PatchError` and `TypeError`: | ||
|
|
||
| ```python | ||
| class NodeTypeError(PatchError, TypeError): | ||
| """Raised when a node is not the expected type for the operation.""" | ||
| ``` | ||
|
|
||
| ### Why dual inheritance? | ||
|
|
||
| - Subclassing `PatchError` preserves backward compatibility: existing `except PatchError` handlers still catch it. | ||
| - Subclassing `TypeError` communicates the semantic meaning: the caller passed a path to the wrong kind of node. | ||
|
|
||
| ### Error Hierarchy After Change | ||
|
|
||
| ``` | ||
| Exception | ||
| ├── TypeError | ||
| │ └── NodeTypeError (also under PatchError) | ||
| └── YAMLTripError | ||
| ├── ParseError | ||
| ├── QueryError | ||
| └── PatchError | ||
| ├── KeyExistsError | ||
| ├── KeyMissingError | ||
| └── NodeTypeError (also under TypeError) | ||
| ``` | ||
|
|
||
| ## Affected Call Sites | ||
|
|
||
| ### Python-side checks (already explicit) | ||
|
|
||
| | Method | Current | New | | ||
| |--------|---------|-----| | ||
| | `remove_from_list()` | `raise PatchError("Value at {keys} is not a list")` | `raise NodeTypeError(...)` | | ||
|
|
||
| ### Rust-side errors (detected by message pattern) | ||
|
|
||
| | Error message pattern | Source operation | New behavior | | ||
| |----------------------|-----------------|--------------| | ||
| | `"Value is not a mapping"` | `add()` on non-mapping | Catch `PatchError`, re-raise as `NodeTypeError` | | ||
| | `"Value is not a sequence"` | `append()` on non-sequence | Catch `PatchError`, re-raise as `NodeTypeError` | | ||
|
|
||
| ### Detection in `_apply_patches` | ||
|
|
||
| Rather than adding detection logic to `_apply_patches` itself (which is generic), each public method that expects a specific node type handles the re-raise: | ||
|
|
||
| ```python | ||
| def append(self, *keys: KeyPart, value: Any) -> Document: | ||
| route = _make_route(keys) | ||
| op = _core.Op.append(value) | ||
| patch = _core.Patch(route=route, operation=op) | ||
| try: | ||
| return self._apply_patches([patch]) | ||
| except PatchError as e: | ||
| if "expected BlockSequence" not in str(e): | ||
| # Not a flow sequence issue — check if it's a type mismatch | ||
| if "not a sequence" in str(e): | ||
| raise NodeTypeError(str(e)) from None | ||
| raise | ||
| # flow sequence fallback (from the other spec) | ||
| ... | ||
| ``` | ||
|
|
||
| ## Error Messages | ||
|
|
||
| The messages stay descriptive of what went wrong: | ||
|
|
||
| ```python | ||
| NodeTypeError("Value at ('config',) is not a list") | ||
| NodeTypeError("Patch failed: Value is not a sequence") | ||
| NodeTypeError("Patch failed: Value is not a mapping") | ||
| ``` | ||
|
|
||
| ## Public API Changes | ||
|
|
||
| ### New export | ||
|
|
||
| ```python | ||
| # yamltrip/__init__.py | ||
| from yamltrip.errors import NodeTypeError | ||
|
|
||
| # yamltrip/errors.py | ||
| class NodeTypeError(PatchError, TypeError): | ||
| """Raised when a node is not the expected type for the operation.""" | ||
| ``` | ||
|
|
||
| ### Stub update | ||
|
|
||
| ```python | ||
| # yamltrip/_core.pyi — no change needed (error is Python-only) | ||
| ``` | ||
|
|
||
| ## Backward Compatibility | ||
|
|
||
| Fully backward compatible: | ||
| - `except PatchError` still catches `NodeTypeError`. | ||
| - `except TypeError` now also catches it (new capability). | ||
| - `except NodeTypeError` is available for callers who want to distinguish. | ||
|
|
||
| ## Not In Scope | ||
|
|
||
| - Checking node types before calling Rust (would require adding Python-side query logic that duplicates Rust). The catch-and-re-raise approach is sufficient. | ||
| - Flow mapping type errors — `sync` on mappings uses `add`/`replace`/`remove` which don't have this problem. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.