Skip to content

Refactor JSON Patch OpRemove and OpMove - #137

Merged
jg-rp merged 2 commits into
mainfrom
op-remove
Oct 17, 2026
Merged

jg-rp merged 2 commits into
mainfrom
op-remove

Conversation

@jg-rp

@jg-rp jg-rp commented Jul 28, 2026 •

Copy link
Copy Markdown
Owner

This PR refactors OpRemove and OpMove to use a common _remove() function for consistency and better code reuse, just like we did with _add() in #136.

OpReplace does not use _remove() due to its distinct behavior when replacing a JSON root value.

@carlitogwapito

Copy link
Copy Markdown

A small optional test-only commit, based directly on this branch.

The shared helper now rejects a move from a MappingProxyType or tuple. The old implementation writes the destination without removing anything from those read-only sources, so it behaves like a copy.

Two small guards worth keeping: reject the read-only source, and leave the destination unchanged on that rejection. This doesn't assume apply() is atomic for other failures.

Checked on Python 3.14.4: both new cases pass your head and catch the old copy-like behavior; the existing move-to-root and array-append tests pass on both. Focused Ruff lint/format passes. I haven't run the full Hatch/compliance suites, typing, build or optional-regex/version/OS matrix.

To run the added file in your tree: python -m pytest tests/test_move_readonly_source.py -q.

@jg-rp

jg-rp commented Oct 17, 2026 •

Copy link
Copy Markdown
Owner Author

Thanks @carlitogwapito.

Yes, this is changing move op semantics and fixing a bug in the old implementation, not just refactoring.

I'll add a test case with an immutable mapping and sequence.

@jg-rp
jg-rp merged commit 7fcebbe into main Oct 17, 2026
28 checks passed
@jg-rp
jg-rp deleted the op-remove branch October 17, 2026 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants