Don't delete nulls inside Arrays

[chrishunt]

Part 3
If the value is either a non-null JSON primitive or an Array and that member
is currently defined within the target resource, the existing value for that
member is to be replaced with that provided. Any object member contained in
the provided data whose value is explicitly null is to be treated as if the
member were undefined

This could go either way, but it reads to me like we want to remove null members, but not necessarily nulls inside of Arrays.

Since Arrays are replaced entirely every time we patch, null doesn't have any special meaning inside an Array; it's just data.

Either way, line #69 can be deleted without failing any tests. We should modify the test in this PR to match whatever behavior we think is best. The implementation in this PR will allow nulls inside Arrays.

[steveklabnik]

We're actually discussing this on the IETF list right now...

On Mon, May 20, 2013 at 5:38 PM, Chris Hunt notifications@github.com
wrote:

> Part 3
> If the value is either a non-null JSON primitive or an Array and that member
> is currently defined within the target resource, the existing value for that
> member is to be replaced with that provided. Any object member contained in
> the provided data whose value is explicitly null is to be treated as if the
> member were undefined
This could go either way, but it reads to me like we want to remove nil members, but not necessarily nils inside of Arrays.
Since Arrays are replaced entirely every time we patch, nil doesn't have any special meaning inside an Array; it's just data.
Either way, line #69 can be deleted without failing any tests. We should modify the test in this PR to match whatever behavior we think is best. The implementation in this PR will allow nils inside Arrays.
You can merge this Pull Request by running:
git pull https://github.com/chrishunt/json-merge_patch chrishunt/dont-delete-nils
Or you can view, comment on it, or merge it online at:
#8
-- Commit Summary --

• Don't delete nils inside Arrays
  -- File Changes --
  M lib/json/merge_patch.rb (23)
  M test/merge_patch_test.rb (4)
  -- Patch Links --
  https://github.com/steveklabnik/json-merge_patch/pull/8.patch
  https://github.com/steveklabnik/json-merge_patch/pull/8.diff

[coveralls]

Coverage remained the same when pulling f81ab81 on chrishunt:chrishunt/dont-delete-nils into 3067a80 on steveklabnik:master.

[chrishunt]

Cool. I'm curious to hear what happens. I tried googling for the list, but there are a TON of lists on http://www.ietf.org and search is failing me.

[steveklabnik]

Ahh look: http://www.ietf.org/mail-archive/web/apps-discuss/current/msg09496.html

On Mon, May 20, 2013 at 5:46 PM, Chris Hunt notifications@github.com
wrote:

Cool. I'm curious to hear what happens. I tried googling for the list, but there are a TON of lists on http://www.ietf.org and search is failing me.

Reply to this email directly or view it on GitHub:
#8 (comment)

[chrishunt]

Thanks for the link. Sounds like it could go either way.

[jasnell]

Recommend keeping this open. The intent has always been to purge all nulls from the final result, obviously if there is consensus from the appsawg to change that, I will do so, but I'd rather not.

[steveklabnik]

Sounds good!

On Mon, May 20, 2013 at 6:47 PM, James M Snell notifications@github.com
wrote:

Recommend keeping this open. The intent has always been to purge all nulls from the final result, obviously if there is consensus from the appsawg to change that, I will do so, but I'd rather not.

Reply to this email directly or view it on GitHub:
#8 (comment)

[chrishunt]

Closing this, it's super old. Re-open later if you want ❤️