Add option to generate test operations in diff

[RobertCochran]

Hi,

This PR adds a new option for generating patches - generate_test_ops - that adds a test operation before every remove and replace operation. This allows for generating patches that have stronger data integrity characteristics. It is also a useful option if you care about knowing what the data was pre-patch (this is important for our use-case). The flag defaults to False to keep the default behavior the same.

I have modified the test suite to run the MakePatchTestCase twice - once with generate_test_ops=True and once with generate_test_ops=False. Patches are still applying properly in both cases so I am confident that all is well.

Please let me know if there's anything else that needs to be done as part of this PR.

Thanks!

[RobertCochran]

Hi, any news on this?

[Copilot]

Pull Request Overview

This PR adds a new generate_test_ops option to the JSON patch generation that creates test operations before every remove and replace operation. This enhances data integrity by capturing the previous values before modifications are applied.

• Adds generate_test_ops parameter to make_patch() and JsonPatch.from_diff() methods with default value False
• Modifies DiffBuilder to generate test operations when the flag is enabled
• Updates test suite to run all existing tests with both generate_test_ops=True and generate_test_ops=False

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.

File	Description
jsonpatch.py	Adds generate_test_ops parameter support and logic to generate test operations in the diff builder
tests.py	Updates all test cases to use the new parameter and adds new test classes to verify behavior with test operations enabled

[Copilot]

These empty methods _on_undo_add and _on_undo_remove in the base PatchOperation class appear to be default implementations that don't perform any logic. Consider adding docstrings to explain their purpose or making them abstract methods if they're meant to be overridden by subclasses.

[Copilot]

The docstring uses 'While :const:True' which is grammatically incorrect. It should be 'When :const:True' or 'If :const:True'.

Suggested change

	:param generate_test_ops: While :const:`True` generate test operations
	capturing previous values of `replace`/`remove`
	operations. By default do not generate these
	:param generate_test_ops: If :const:`True`, generate test operations
	capturing previous values of `replace`/`remove`
	operations. By default, do not generate these

[Copilot]

Same grammatical error as in the make_patch function. The docstring uses 'While :const:True' which should be 'When :const:True' or 'If :const:True'.

Suggested change

	:param generate_test_ops: While :const:`True` generate test operations
	:param generate_test_ops: If :const:`True`, generate test operations

[Copilot]

[nitpick] The code accesses prev_op_index[2] directly using a magic index. Consider using a named constant or a more descriptive way to access the operation object to improve code readability.

[Copilot]

[nitpick] The expected result is defined before the conditional logic that modifies it. Consider moving the base expectation definition closer to where it's used, or make the conditional logic more explicit about what's being added.

Suggested change

	exp = [{'op': 'remove', 'path': '/foo/0/baz'}]
	if self.generate_test_ops:
	exp.insert(0, {'path': '/foo/0/baz', 'value': 2, 'op': 'test'})
	if self.generate_test_ops:
	exp = [
	{'path': '/foo/0/baz', 'value': 2, 'op': 'test'},
	{'op': 'remove', 'path': '/foo/0/baz'}
	]
	else:
	exp = [{'op': 'remove', 'path': '/foo/0/baz'}]