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
