Closed eriksunsol closed 5 years ago
Tests added. I have also worked through no-change operations on array elements and made sure undefined
and null
are treated properly with respect to their JSON projections in objects and arrays (which is slightly different). I took the liberty to add a test or two which are variants of tests that already existed, but for the special cases e.g. when an undefined
property is set to null
. This is to make sure testing for no-change operations do not catch cases which should be left alone.
@warpech suggested to make return Reflect.set(tree, key, newValue);
DRYier
I will make the required changes in a new PR.
@warpech suggested to make return Reflect.set(tree, key, newValue); DRYier
@tomalec I have made two alternative solutions:
Reflect.set
before the big pile of IFs with many return statements: https://github.com/Palindrom/JSONPatcherProxy/commit/43f3439dd5b43a1d9eb7f348074b3f76c4958e8cReflect.set
after the big pile of IFs with single return statement: https://github.com/Palindrom/JSONPatcherProxy/commit/71078c341ad83d8313c8c13a65178f5f9ba8ece6The code is quite complex, and I am not very happy with the readability of either.
Any suggestions which is better? (I can accept "Current" as an answer, too)
I have a slight preference over 1. because of readable variable names wasKeyInTreeBeforeReflection, valueBeforeReflection
and the fact that whether the callback/following code is executed is controlled using standard return
, not enigmatic operation.op
As discussed today between @eriksunsol, @tomalec and me, I have pushed my solution that fixes the test created by @eriksunsol (https://github.com/Palindrom/JSONPatcherProxy/pull/45/commits/0257a923ad761dc0ec9507c869039f57d7142170).
@tomalec, @eriksunsol could you pls check and give your blessing?
I plan to add more tests from https://github.com/Starcounter-Jack/JSON-Patch/tree/master/test in a separate PR before next release.
We found another issue with this function. Is it called when you
pop
an array? Then change fromundefined
(mapped tonull
) to not existing element. I guess it should calldeleteTrap
but it's worth to add a test for such case as well.
Let's continue that in https://github.com/Palindrom/JSONPatcherProxy/issues/52
Extending the example in README.md like this:
results in the following patch:
The second and third time
firstName
is set are effectively no-ops which have already been communicated. I think this is wrong. The proposed change recognizes this and renders the following result instead: