search

Starcounter-Jack / JSON-Patch

Lean and mean Javascript implementation of the JSON-Patch standard (RFC 6902). Update JSON documents using delta patches.
MIT License
1.79k stars 215 forks source link

Add ability to generate test operations #226

Closed pytlesk4 closed 5 years ago

pytlesk4 commented 5 years ago

Implements #211

When observing an object, or comparing two objects if inversible is true, then a test operation will be generated for replace/remove operation.

alshakero commented 5 years ago

Hi! Thank you so much for the effort. I wish you asked in the issue first.

This library is meant to be as fast as possible. And adding this feature will have some impact that I'm not sure worth it. The final say is for @tomalec and @warpech though.

Plus it's pretty hard to review the PR with all the formatting changes. It would be easier if your PR included as few changes as possible, preferably only your logic changes. And the maintainer should take care of the formatting.

pytlesk4 commented 5 years ago

Hi! Thank you so much for the effort. I wish you asked in the issue first.

This library is meant to be as fast as possible. And adding this feature will have some impact that I'm not sure worth it. The final say is for @tomalec and @warpech though.

Plus it's pretty hard to review the PR with all the formatting changes. It would be easier if your PR included as few changes as possible, preferably only your logic changes. And the maintainer should take care of the formatting.

Sorry for not asking, that is my fault.

I undid all the formatting, should be a lot easier to review. I initially forgot to turn off auto format on save in vscode.

The performance should be the same though, by default this doesn't do anything different. You have to turn this feature on. Performance in theory should be pretty much the same, even if it is turned on, the hit should be pretty small/barely noticeable.

tomalec commented 5 years ago

Thanks for the effort of providing really nice PR, with tests, docs, and good code.

As @alshakero said, we are pretty paranoid on performance here.

Could you elaborate on actual use-case for this change? So I could get better understanding on rationale behind?

pytlesk4 commented 5 years ago

@tomalec test operations are needed to generate the inverse of a JSON Patch, similar to what jiff is doing here -> https://www.npmjs.com/package/jiff#diff

I ran the benchmarks on my branch on my machine, and here are the results. This is not generating any test operations. Will update the benchmarks to see what the performance diff is.

image

Here are the results when generating test operations. I am committing the benchmarks now.

image

Just for reference, I benched master as well.

image
pytlesk4 commented 5 years ago

Bump

warpech commented 5 years ago

I am working on accepting this PR, however I have few questions:

  1. Is this OK, if I remove the changes in the constructor, and only make a boolean attribute for generate and compare methods.
  2. Shouldn't this be called invertible instead of inversible? Sorry, I'm not native, but invertible seems a more popular word
  3. https://github.com/immerjs/immer also registers inverse patches, but they do it in a separate patch, not within the same patch. Is your solution more useful for you? Maybe we should do the same as Immer did instead?
warpech commented 5 years ago

Closing in favor of PR #228