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

> > At the moment I really dislike using **kwargs as shorthand for a dict
> > to be passed to lower level code, eg to be passed in to string
> > interpolating. It makes things slightly easier on the caller it's
> > true, but it also tends to break by clashing with other arguments used
> > either at this level or in different implementations or as the code
> > changes later on.
>
> I see. Well, turning the callerrs x=2, y=3 into dict(x=2, y=3) won't
> such a problem. But in this case, the migration from one to the other
> should occur sooner rather than later, keeping different idioms in the
> code base... is painful in the long run.

If you're talking about going back to clean up all existing cases like this, then it is a bit painful. I think not adding any more of them is worthwhile.

I think the coding guideline would be, don't mix kwargs where the caller could want provide any arbitrary value with other parameters. Use kwargs only for metaprogramming. Perhaps this is too harsh and needlessly closes off a useful tool though.

> >> Oh wait, yes it *is* used in other implementations, ok.
> >>
> >> Hmm, I think that 'confirm' already implies that the default is
> >> True so this may be useless. Do you have a use case for
> >> default=False ?
>
> > That would imply an operation where we want confirmation from a user
> > and we won't proceed in non-interactive mode, which is conceivable but
> > unlikely. So perhaps I should just delete it.
>
> Then we could introduce deny_action when needed ?

If we think we'll want it, perhaps we should keep the option now?

> > So bzrlib.lockdir.break, or something similar?
>
> At least internally, but the caller itself may use an aliased form.

I think having aliases for this might just cause confusion. My updated branch shows some suggested names.