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