On 2019-Sep-13, Fabien COELHO wrote:
> Hello Alvaro,
>
> > I think the TestLib.pm changes should be done separately, not together
> > with the rest of the hacking in this patch.
> >
> > Mostly, because I think they're going to cause trouble. Adding a
> > parameter in the middle of the list may cause trouble for third-party
> > users of TestLib.
>
> That is also what I thought, however, see below.
I see. But you seem to have skipped my suggestion without considering
it.
I think the current API of these functions where they just receive a
plain array of arguments, and all callers have to be patched in unison,
is not very convenient. Also, I *think* your new icommand_checks method
is the same as command_checks_all, except that you also have the "init"
part. So you're duplicating code because the original doesn't have
functionality you need? But why do that, if you could have *one*
function that does both things? If some callers don't have the "init"
part, just omit it from the parameters.
(Whether it's implemented using Expect or not should not matter. Either
Expect works everywhere, and we can use it, or it doesn't and we can't.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
