Code review comment for lp:~mbp/bzr/ui-factory

Regarding this patch: I'm in favour of it. There's a nit that default=True still exists in the signature of one of the implementations, so Needs Fixing.

I'm not worried about the slight inconvenience of an explicit dict rather than **kwargs. There just aren't enough places where we ask users to confirm an action for this to be a big issue. And I do like the explicit separation of function params vs. string interpolation params.

Regarding the general guidelines for kwargs: I think it should be used sparingly. I think it can be good when a function is delegating largely to another callable (e.g. it is some sort of higher order function, like TestCase.addCleanup). Otherwise I tend to be sceptical. If the parameters to a function are a bit complex and might vary over time (e.g. the commit API) then I think what's really called for is an explicit object for the parameters (e.g. I think we should create a CommitParams object), rather than a bag of positional and/or keyword args. If you have an arbitrary set of keys and values that are different with each use (e.g. string interpolation inputs) then again I think that should not be mixed in with the regular positional/keyword args, it seems like a different category of thing.

I guess a short rule-of-thumb is: be wary of any **kwargs that aren't accompanied by a param called 'callable'.