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

>>>>> Martin Pool <email address hidden> writes:

    >> > 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.

It will be painful *once*, but less painful for new contributors that
won't wonder which one to use, for reviewers who won't have to say "you
picked the wrong one", etc. Just saying ;)

<snip/>

    >> > 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?

confirm_action(default=False) sounds really akward compared to
deny_action()...

    >> > 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.

Could be, let's see how it's used (but we already have many conf vars
that could benefit to be associated with a python name and then we'll
have to ensure backward compatibility anyway).