On Thu, Aug 7, 2025 at 9:35 AM Dagfinn Ilmari Mannsåker
<ilm...@ilmari.org> wrote:
> I haven't read the meat of the patch, but I have some comments on the
> tests:
Thanks for the review!
> > +IPC::Run::run ['oauth_tests'],
> > + '>', IPC::Run::new_chunker, sub { print {$out} $_[0] },
> > + '2>', IPC::Run::new_chunker, sub { print {$err} $_[0] }
> > + or die "oauth_tests returned $?";
>
> We've recently switched to using fat commas (=>) between options and
> their arguments, and that includes the file redirections in IPC::Run.
> Although not semantically meaningful, I'd also be tempted to put parens
> around the argument list for each redirect, so it's clear that they go
> together.
I have two concerns:
- If I don't put parentheses around the list, the fat comma is
actively misleading.
- As far as I can tell, IPC::Run neither documents nor tests the
ability to pass a list here. (But the tests are a bit of a maze, so
please correct me if there is one.) My fear is that I'll be coupling
against an implementation detail if I write it that way.
So I'm leaning towards keeping it as-is, unless you know of a reason
that the list syntax is guaranteed to work, with the understanding
that it does diverge from what you authored in 19c6e92b1. But I don't
think any of those examples use filters, so I don't feel too bad about
the difference yet?
> Also, indirect object syntax (print {$fh} ...) is ugly and
> old-fashioned, it's nicer to call it as a method on the filehandle.
That is much nicer; I'll do that.
> As for the C TAP tests, there's already a bunch of TAP-outputting
> infrastructure in pg_regress.c. Would it make sense to factor that out
> into a common library?
Maybe if we got to rule-of-three, but I'd rather not make either
implementation compromise for the sake of the other. IMO, this is a
situation where a bad abstraction would be much costlier than the
duplication: TAP is lightweight, and I think the needs of a unit test
suite and the needs of a characterization test collector are very
different.
--Jacob
