Re: new json funcs

On 01/28/2014 05:07 PM, Marko Tiikkaja wrote:
> Hi Andrew,
>
> On 1/24/14, 7:26 PM, Andrew Dunstan wrote:
>> OK, here's the patch, this time with docs, thanks to Merlin Moncure and
>> Josh Berkus for help with that.
>
> Thanks, this one is looking pretty good.  A couple of small issues:
>
>   - The oid 3195 of json_object_agg_transfn has been taken by a recent 
> commit, so that had to be changed.  The patch compiled and passed 
> tests after that.

Yeah. These days you can't even build if there's a duplicate oid, so 
fixing that and a catalog version bump would be part of committing.

>
>   - Typo in the description of json_build_array: "agument list"

will fix.

>
>   - I find (perhaps due to not being a native speaker) the description 
> of json_object a bit painful to read.  I would've expected something 
> like:
>
> -         Builds a JSON object out of a text array.  The array must 
> have exactly one dimension
> +         Builds a JSON object out of a text array.  The array must 
> have either exactly one dimension
>           with an even number of members, in which case they are taken 
> as alternating name/value
> -         pairs, or two dimensions with such that each inner array has 
> exactly two elements, which
> +         pairs, or two dimensions such that each inner array has 
> exactly two elements, which
>           are taken as a name/value pair.
>
>      but I'm not sure about that either.

Yes, yours looks better.

>
>   - There are a few cases of curly braces around a single-statement 
> else, which I believe is against the project's code style guidelines.


IIRC we actually stopped pgindent removing that quite a few years ago, 
and the formatting guidelines at 
<http://www.postgresql.org/docs/devel/static/source-format.html> don't 
say anything about it. Personally, I prefer consistency - I think either 
both branches of an if/else should use curly braces or neither should. I 
find it quite ugly and jarring when one does and the other doesn't.


Thanks for the review.

cheers

andrew