On 11/8/19 11:25 AM, Mark Dilger wrote: > > > 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.
This will need to be rewritten in light of the above, but see <https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a...@2ndquadrant.com> cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
