Re: TestLib::command_fails_like enhancement

On 11/8/19 6:33 AM, Andrew Dunstan wrote:
> 
> On 11/8/19 1:16 AM, Craig Ringer wrote:
>> On Fri, 8 Nov 2019 at 06:28, Mark Dilger <hornschnorter@gmail.com
>> <mailto:hornschnorter@gmail.com>> wrote:
>>
>>
>>
>>      On 10/31/19 10:02 AM, Andrew Dunstan wrote:
>>      >
>>      > This small patch authored by my colleague Craig Ringer enhances
>>      > Testlib's command_fails_like by allowing the passing of extra
>>      keyword
>>      > type arguments. The keyword initially recognized is
>>      'extra_ipcrun_opts'.
>>      > The value for this keyword needs to be an array, and is passed
>>      through
>>      > to the call to IPC::Run.
>>
>>      Hi Andrew, a few code review comments:
>>
>>      The POD documentation for this function should be updated to
>>      include a
>>      description of the %kwargs argument list.
>>
>>      Since command_fails_like is patterned on command_like, perhaps you
>>      should make this change to both of them, even if you only originally
>>      intend to use the new functionality in command_fails_like.  I'm
>>      not sure
>>      of this, though, since I haven't seen any example usage yet.
>>
>>      I'm vaguely bothered by having %kwargs gobble up the remaining
>>      function
>>      arguments, not because it isn't a perl-ish thing to do, but
>>      because none
>>      of the other functions in this module do anything similar.  The
>>      function
>>      check_mode_recursive takes an optional $ignore_list array
>>      reference as
>>      its last argument.  Perhaps command_fails_like could follow that
>>      pattern
>>      by taking an optional $kwargs hash reference.
>>
>>
>> Yeah, that's probably sensible.
>>
>>
>>
> 
> 
> OK, I will rework it taking these comments into account. Thanks for the
> comments Mark.

I'd be happy to see the regression tests you are writing sooner than 
that, if you don't mind posting them.  It's hard to do a proper review 
for you without a better sense of where you are going with these changes.

-- 
Mark Dilger