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 <hornschnor...@gmail.com
<mailto:hornschnor...@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
