feat: add RoutingError for non-mapping route failures#48
Merged
Conversation
nathanjmcdougall
changed the title
The previous loop iterated _PatchErrorKind in enum definition order, making the ordering load-bearing but invisible to future maintainers. Replace with an explicit _ordered tuple inside the function. The tuple literal documents that ordering is intentional: more-specific substrings must precede any shorter ones that are a prefix of them. Adding a new kind now requires a deliberate insertion at the right position, not just a new enum member.
Add two missing cases to TestRoutingErrorBehaviour: - test_add_through_list_raises_routing_error: add() through a list node was untested; only the scalar-node case existed. - test_upsert_scalar_root_raises_routing_error: exercises the _create_at(parent_keys=(), ...) branch on a non-empty document and pins the '<root>' token in RoutingError messages.
- test_merge_through_list_raises: pins RoutingError when merge() routes through a list node (complements existing scalar test). - test_sync_through_scalar_raises_routing_error: pins that sync() propagates RoutingError via its upsert delegation path.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a dedicated RoutingError exception to represent “route passes through a non-mapping node” failures during write operations, allowing callers to reliably distinguish routing problems from other patch/type errors without matching Rust-layer error strings.
Changes:
- Introduces
RoutingError(PatchError)and exports it via the package public API (__init__.py/__all__). - Updates
Document.add()/_create_at()(and thereforeupsert(),sync(), andmerge()’s missing-path delegation) to re-raise routing-related patch failures asRoutingErrorwith a stable, human-readable message. - Extends the internal patch-error classifier with two new Rust substring pins and updates tests to assert the new exception type and behavior (including
merge()now raisingRoutingErrorfor these cases).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sync.py | Adds a regression test asserting sync() surfaces routing failures as RoutingError. |
| tests/test_public_api.py | Ensures RoutingError is part of the public API contract (subclassing + export). |
| tests/test_merge.py | Updates merge() routing-failure expectations to RoutingError and adds list-routing coverage. |
| tests/test_errors.py | Adds hierarchy and behavior tests for RoutingError across upsert()/add() scenarios. |
| tests/test_edge_cases.py | Pins new Rust-layer substrings and validates classification for routing-related failures. |
| src/yamltrip/errors.py | Defines the new RoutingError(PatchError) type. |
| src/yamltrip/document.py | Adds routing error kinds + helper, refactors classifier, and re-raises routing failures as RoutingError from add() / _create_at(); simplifies merge(). |
| src/yamltrip/init.py | Exports RoutingError and includes it in __all__. |
| doc/specs/2026-05-26-routing-error-design.md | Documents the design, detection strategy, raise sites, and breaking-change behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Introduces
RoutingError(PatchError)— a typed exception raised when a key path passes through a non-mapping node (scalar or list) duringadd(),upsert(), ormerge().Previously these failures surfaced as a bare
PatchError(or, inmerge(), aNodeTypeErrorwith a hand-crafted message). Callers had no way to distinguish routing failures from other patch errors without string-matching Rust internals.Changes
errors.pyRoutingError(PatchError)— placed betweenKeyMissingErrorandNodeTypeError. NoTypeErrormixin.document.py_PatchErrorKind: two new members —NON_MAPPING_ROUTEandEXPECTED_MAPPING._classify_patch_error: refactored to a loop (fixes PLR0911)._is_routing_error(kind): helper returningTruefor all three routing-related kinds._create_atandadd(): routing errors re-raised asRoutingErrorwith messageRoute passes through a non-mapping node at <path>.merge(): drops the ancestor-hunting try/except —RoutingErrorpropagates fromupsert → _create_at.__init__.pyRoutingErroradded to imports and__all__.Tests
test_errors.py:TestRoutingErrorHierarchyandTestRoutingErrorBehaviour.test_edge_cases.py: four newTestPatchErrorStringPinsentries for the two new Rust substrings.test_merge.py: updated to expectRoutingError.test_public_api.py:test_error_classesextended withRoutingError.Breaking change
doc.merge("a", "b", "c", value=...)wherea.bis a scalar previously raisedNodeTypeError; it now raisesRoutingError. Both arePatchErrorsubclasses, so code catchingPatchErroris unaffected.Spec
doc/specs/2026-05-26-routing-error-design.md