RE: remaining sql/json patches

Hi,
Thank you for developing such a great feature. The attached patch formats the documentation like any other function definition:
- Added right parenthesis to json function calls.
- Added <returnvalue> to json functions.
- Added a space to the 'expression' part of the json_scalar function.
- Added a space to the 'expression' part of the json_serialize function.

It seems that the three functions added this time do not have tuples in the pg_proc catalog. Is it unnecessary?

Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Amit Langote <amitlangote09(at)gmail(dot)com>
Sent: Wednesday, July 26, 2023 5:10 PM
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>; Erik Rijkers <er(at)xs4all(dot)nl>; PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>; jian he <jian(dot)universality(at)gmail(dot)com>
Subject: Re: remaining sql/json patches

On Fri, Jul 21, 2023 at 7:33 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Fri, Jul 21, 2023 at 1:02 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > On 2023-Jul-21, Amit Langote wrote:
> >
> > > I’m thinking of pushing 0001 and 0002 tomorrow barring objections.
> >
> > 0001 looks reasonable to me. I think you asked whether to squash
> > that one with the other bugfix commit for the same code that you
> > already pushed to master; I think there's no point in committing as
> > separate patches, because the first one won't show up in the
> > git_changelog output as a single entity with the one in 16, so it'll
> > just be additional noise.
>
> OK, pushed 0001 to HEAD and b6e1157e7d + 0001 to 16.
>
> > I've looked at 0002 at various points in time and I think it looks
> > generally reasonable. I think your removal of a couple of newlines
> > (where originally two appear in sequence) is unwarranted; that the
> > name to_json[b]_worker is ugly for exported functions (maybe "datum_to_json"
> > would be better, or you may have better ideas);
>
> Went with datum_to_json[b]. Created a separate refactoring patch for
> this, attached as 0001.
>
> Created another refactoring patch for the hunks related to renaming of
> a nonterminal in gram.y, attached as 0002.
>
> > and that the omission of
> > the stock comment in the new stanzas in FigureColnameInternal() is
> > strange.
>
> Yes, fixed.
>
> > But I don't have anything serious. Do add some ecpg tests ...
>
> Added.
>
> > Also, remember to pgindent and bump catversion, if you haven't already.
>
> Will do. Wasn't sure myself whether the catversion should be bumped,
> but I suppose it must be because ruleutils.c has changed.
>
> Attaching latest patches. Will push 0001, 0002, and 0003 on Monday to
> avoid worrying about the buildfarm on a Friday evening.

And pushed.

Will post the remaining patches after addressing jian he's comments.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com