search

JSON8 / patch

Moved to https://github.com/sonnyp/JSON8/tree/master/packages/patch
ISC License
20 stars 2 forks source link

Can't a {reversible: true} revert just be a patch? #9

Closed thesunny closed 8 years ago

thesunny commented 8 years ago

When you call apply with {reversible: true} it returns an object that is different than a patch.

var patchResult = ooPatch.apply(doc, patch, {reversible: true};
patchResult.revert // this does not return a patch object but a custom revert object

I'm worried about the semantics of the patchResult being something other than a patch. Couldn't this just be a normal patch?

I was working on something similar and I was returning patches for reversibles. Is there any reason why patchResult.revert doesn't just return a patch? Is this a design decision?

sonnyp commented 8 years ago

Good question. As far as I can remember there is no good reason for it not being a patch. I'll have a look.

sonnyp commented 8 years ago

Also I don't like the API, calling apply with a reversible option and returning 2 values. Any idea? Maybe applyAndReversible ?

sonnyp commented 8 years ago

https://github.com/JSON8/patch/pull/10

thesunny commented 8 years ago

Wow, that was an amazingly fast response sonnyp. Amazing.

I agree with you about the calling semantics. It really feels like apply should always return a Doc with the patches applied instead of an Object that contains the doc and a patch.

Your suggestion sounds good although I also might consider using something completely different like ooPatch.reply(doc, patch). I don't know if that name is good but it was (a) different so as not to be confused with apply (b) short and (c) kind of meshes reversible and apply and it kind of is a patch with a reply and the same time.

thesunny commented 8 years ago

This will be quite nice as well because the reverts can also be compressed now.

sonnyp commented 8 years ago

https://github.com/JSON8/patch/pull/15

sonnyp commented 8 years ago

Closing, let's move discussion to https://github.com/JSON8/patch/pull/15