Repository navigation
Support whole document operations on scalar documents (#213) - #219
Merged
Merged
Conversation
A JSON document can be a scalar, but operations on the whole document (path "") raised a bare TypeError for one, which is not part of the documented exceptions: - 'add' (and 'copy', which goes through it) to "" now replaces the document whatever its type, as 'replace' already did. - 'remove' of "" now raises JsonPatchConflict for every type of document, instead of a TypeError for scalars and a misleading "non-existent object 'None'" / "invalid array index 'None'" for objects and arrays. - 'move' from "" resolves the whole document as 'copy' does, so moving it onto itself is a no-op and moving it elsewhere fails with JsonPatchConflict. This also removes the TypeError for array documents from #214; its check for moves into a location's own children is still missing. The TypeError branches in 'replace' could no longer be reached and are removed. test_whole_document_as_target now holds; test_raises_only_documented_exceptions still fails, only because of #215. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01964EYaTCKUya9tcPjhytDq
master now rejects moves into a location's own children (#214), which also covers 'move' from "". Resolved by keeping master's MoveOperation: it returns early when 'from' is "", so the whole-document branch this branch had added there is dropped as unreachable. The comment on the 'move' from "" example keeps master's reference to #214. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01964EYaTCKUya9tcPjhytDq
master fixes #215, which drops the 'copy' from '/-' example from test_raises_only_documented_exceptions. Resolved by taking that removal and keeping this branch's comment on the scalar example. With #213, #214 and #215 all fixed, the test now holds, so its expectedFailure marker goes too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01964EYaTCKUya9tcPjhytDq
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
2 participants
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.
Fixes #213.
A JSON document can be a scalar (
null, a number, a string or a boolean), but operations on the whole document (path"") raised a bareTypeErrorfor one. That is not part of the documented exceptions, soexcept jsonpatch.JsonPatchExceptiondidn't catch it.Changes
addto""now replaces the document whatever its type, asreplacealready did.copyto""goes through the same code and is fixed with it.removeof""now raisesJsonPatchConflict("can't remove the whole document")for every type of document. Before, scalars raised aTypeError, and strings and arrays raisedJsonPointerException("invalid array index 'None'"). Objects already raisedJsonPatchConflict, but with the misleading message "can't remove a non-existent object 'None'".replace: removes theTypeError("invalid document type ...")branches, which could no longer be reached.movefrom""is not changed here. #214 already handles it on master, and it works for scalar documents too: moving""onto itself does nothing, and moving it anywhere else raisesJsonPatchConflict. The new tests cover that case as well.Behaviour change:
removeof""on an array (or a string) now raisesJsonPatchConflictrather thanJsonPointerException. Both are documented exceptions, but code that catches onlyJsonPointerExceptionfor this case would no longer catch it.Tests
tests.py: new tests coveradd,replace,copyandmoveon""forNone,True,5,1.5and'foo', andremoveof""for those plus an object and an array.property_tests.py: removes theexpectedFailuremarker fromtest_whole_document_as_target. It also removes the marker fromtest_raises_only_documented_exceptions, which now holds because Operations on the whole document raise TypeError when the document is a scalar #213, move does not reject moving an array element into its own children, and crashes when moving the whole array #214 and Malformed patches and "from": "/-" raise TypeError instead of InvalidJsonPatch / JsonPointerException #215 are all fixed. No expected failures remain inproperty_tests.py.tests.pypasses 184 tests,property_tests.pypasses 19, and the doctests pass. The two tests whose markers were removed also passed withHYPOTHESIS_PROFILE=thorough, which runs 5000 examples.🤖 Generated with Claude Code
https://claude.ai/code/session_01964EYaTCKUya9tcPjhytDq