Hi Alvaro,
Thanks for taking a look.
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.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com