On Sun, Dec 18, 2016 at 1:27 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote: > Speaking about implementation of `jsonb_delete_array` - it's fine, but I > would like to suggest two modifications: > > * create a separate helper function for jsonb delete operation, to use it in > both `jsonb_delete` and `jsonb_delete_array`. It will help to concentrate > related logic in one place.
I am not sure that it is much a win as the code loses readability for a minimal refactoring. What would have been nice is to group as well jsonb_delete_idx but handling of the element deletion is really different there.
I agree with that. I agree with investigating it as an option, but I think the lost readability is worse.
> * use variadic arguments for `jsonb_delete_array`. For rare cases, when > someone decides to use this function directly instead of corresponding > operator. It will be more consistent with `jsonb_delete` from my point of > view, because it's transition from `jsonb_delete(data, 'key')` to > `jsonb_delete(data, 'key1', 'key2')` is more smooth, than to > `jsonb_delete(data, '{key1, key2}')`.
That's a good idea.
I can see the point of that. In the particular usecase I built it for originally though, the list of keys came from the application, in which case binding them as an array was a lot more efficient (so as not to require a whole lot of different prepared statements, one for each number of parameters). But that should be workaround-able using the VARIADIC keyword in the caller. Or by just using the operator.
> I've attached a patch with these modifications. What do you think?
Looking at both patches proposed, documentation is still missing in the list of jsonb operators as '-' is missing for arrays. I am marking this patch as waiting on author for now.
Added in updated patch. Do you see that as enough, or do we need it in some more places in the docs as well?