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

Add ability to generate test operations #226

Closed
wants to merge 12 commits into from
Closed

Add ability to generate test operations #226

wants to merge 12 commits into from

Conversation

pytlesk4
Copy link
Contributor

@pytlesk4 pytlesk4 commented May 8, 2019

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

tomalec commented May 13, 2019

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
Copy link
Contributor Author

pytlesk4 commented May 13, 2019

@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
Copy link
Contributor Author

Bump

@warpech
Copy link
Collaborator

warpech commented Jul 2, 2019

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
Copy link
Collaborator

warpech commented Jul 2, 2019

Closing in favor of PR #228

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

Successfully merging this pull request may close these issues.

None yet

4 participants