Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement conversion to JSON Patch format #15

Open
martinal opened this issue Dec 17, 2015 · 6 comments
Open

Implement conversion to JSON Patch format #15

martinal opened this issue Dec 17, 2015 · 6 comments

Comments

@martinal
Copy link
Collaborator

As mentioned here:
jupyter/enhancement-proposals#8 (comment)

JSON Patch is a standardized format for representing patches to json documents. We should at least support converting to it, if not adapting it internally as well (it's a bit more verbose).

@martinal
Copy link
Collaborator Author

An implementation of json patch conversion is available as

    json_patch_obj = nbdime.diff_format.to_json_patch(nbdime_diff_obj)

However I'm not sure what the correct way is to insert multiple values before the same index. The json patch spec seems to be a bit underspecified, could look at other tools to find what they do.

@bollwyvl
Copy link

Great work going on, and on all the other PRs/Issues! I just pulled, and will check it out!

correct way is to insert multiple values before the same index

I don't think it's possible, explicitly, and would just require multiple adds:

import jsonpatch
import json
a = [1,2,3]
b = [1,2,"orange", "blue", 3]
patch = jsonpatch.make_patch(a, b)
patch.patch
[
  {
    "value": "orange",
    "path": "/2",
    "op": "add"
  },
  {
    "value": "blue",
    "path": "/3",
    "op": "add"
  }
]

Verbose? Sure. But in its verbosity, it requires a far simpler mental model... we shall see if i am more effective building things on this than on straight up dime diff format!

@martinal
Copy link
Collaborator Author

It's not the verbosity I have problems with, it's what the index in the path refers to. The path /3 here refers to index 3 of the intermediate json object that results from applying the first operation. In other words indices in jsonpatch paths are moving targets... I don't agree that this is a simpler mental model :)

In nbdime, the indices in the diff refer to positions in the base object, and this means what you get from the current implementation of to_json_patch in nbdime is garbage, as the indices will need to be updated.

@martinal
Copy link
Collaborator Author

Really, I don't understand the utility of this model... If you want to see which cells have been modified, which lines have been modified, etc., then the indices in the nbdime diff will be what you want. The indices in the jsonpatch paths are can only be interpreted by going through the patch process.

But thanks for the input!

@bollwyvl
Copy link

The path /3 here refers to index 3 of the intermediate json object that results from applying the first operation.

I'm getting my head around it right now!

Really, I don't understand the utility of this model... If you want to see which cells have been modified, which lines have been modified, etc., then the indices in the nbdime diff will be what you want.

Again, recall my user story:

As a viewer of a notebook, I want to visualize the difference between it's rendered DOM and that of two or more notebooks

Hopefully i'll have a [wip] PR up soon, which will help me understand more of what's going on.

@martinal
Copy link
Collaborator Author

I believe to_json_patch is now working, however the unit test is extremely short!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants