-
Notifications
You must be signed in to change notification settings - Fork 215
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
Add ability to generate test operations #226
Conversation
- add inversible property to IObserver - update readme
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. |
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? |
@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. Here are the results when generating test operations. I am committing the benchmarks now. Just for reference, I benched master as well. |
Bump |
I am working on accepting this PR, however I have few questions:
|
Closing in favor of PR #228 |
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.