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@gmail.com> 
Sent: Wednesday, July 26, 2023 5:10 PM
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Andrew Dunstan <andrew@dunslane.net>; Erik Rijkers <er@xs4all.nl>; PostgreSQL-development
<pgsql-hackers@postgresql.org>;jian he <jian.universality@gmail.com>
 
Subject: Re: remaining sql/json patches

On Fri, Jul 21, 2023 at 7:33 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Jul 21, 2023 at 1:02 AM Alvaro Herrera <alvherre@alvh.no-ip.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