refactor: centralise patch-error classification and apply stepdown rule#47
Merged
nathanjmcdougall merged 2 commits intoMay 25, 2026
Merged
Conversation
Add `_PatchErrorKind` enum and `_classify_patch_error()` to confine all yamlpatch error-message substrings in one place. Refactor `append`, `insert`, `extend_list`, and `sync` to branch on the enum rather than matching raw strings directly. Add `TestPatchErrorStringPins` to `test_edge_cases.py` to pin each raw yamlpatch substring against a live Rust error so any upstream change to the error messages is caught immediately. Apply stepdown rule throughout `document.py` and `editor.py`: - Move `_apply_patches`/`_from_core` to after `find_index` - Move `_create_at`/`_is_empty_document` to after `upsert` - Swap `prune_remove`/`remove` so the delegating wrapper comes first - Move module-level private helpers to after the `Document` class - Move `Editor.document` property to after all delegation methods
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the Python Document patch-application error handling to centralize Rust/yamlpatch error-message substring matching into a single classifier, and reorders definitions in document.py / editor.py to follow the “stepdown rule” (callers before callees).
Changes:
- Added
_PatchErrorKind+_classify_patch_error()and updatedappend,insert,extend_list, andsyncto branch on the enum instead of inline string matching. - Reordered methods/helpers in
document.pyand movedEditor.documentproperty to follow all its callers. - Added edge-case tests that trigger Rust patch errors and assert the expected pinned substrings / classification behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_edge_cases.py | Adds tests to pin Rust/yamlpatch error substrings and validate _classify_patch_error() behavior. |
| src/yamltrip/editor.py | Reorders document property to follow its callers (stepdown rule), no functional change. |
| src/yamltrip/document.py | Centralizes patch-error classification and refactors sequence mutation fallbacks to use it; reorders definitions for stepdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Document.merge() was still branching on the raw string 'unexpected node' inline. Add _PatchErrorKind.UNEXPECTED_NODE = 'unexpected node' and the corresponding check in _classify_patch_error(), then update merge() to use the classifier. Also add two new TestPatchErrorStringPins tests: one pinning the raw substring (triggered by Op.merge_into on a scalar node) and one verifying the classifier round-trips correctly.
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
Confines all yamlpatch error-message substrings in one place and enforces the stepdown rule throughout the Python layer.
Centralised error classification
Adds
_PatchErrorKind(enum) and_classify_patch_error()(module-level helper afterDocument) so thatappend,insert,extend_list, andsyncbranch on a typed value instead of matching raw strings inline.Adds
TestPatchErrorStringPinsintest_edge_cases.py— seven tests that trigger each error path directly against the Rust extension and assert the expected substring is present. Any upstream change to yamlpatch error messages will fail these tests immediately, making the single point of containment visible and testable.Stepdown rule (
document.pyandeditor.py)Reorders definitions so callers appear above their callees at every level:
document.py— in class:_apply_patches/_from_coremoved to afterfind_index(previously right after__init__, before all public callers)upsertmoved to before_create_at/_is_empty_documentprune_removeswapped to beforeremove(it delegates toremove)document.py— module level:_ensure_str_keys,_flow_seq_replacements,_PatchErrorKind,_classify_patch_errormoved to after theDocumentclass (they are called byDocumentmethods, soDocumentis the higher-level abstraction)editor.py:documentproperty moved from afteroriginalto afterextract— every delegating method callsself.document, so it now follows all its callers