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. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
