From: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
---|---|
To: | Marko Tiikkaja <marko(at)joh(dot)to> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: new json funcs |
Date: | 2014-01-28 22:37:19 |
Message-ID: | 52E8311F.702@dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | David Fetter | 2014-01-28 22:38:33 | Re: Changeset Extraction v7.3 |
Previous Message | Rod Taylor | 2014-01-28 22:31:25 | Re: Changeset Extraction v7.3 |