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

117 + def confirm_action(self, prompt, confirmation_id, args, default=True):

s/args/kwargs/

or even **kwargs like the others, there should be a way to make
this compatible with 'default=True' (which isn't documented nor
used by the way).

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 ?

Adding confirm_action to uis sounds like a good idea, but this
needs to be discussed with the qbzr/bzr-gtk people since they
need a way to override it without calling get_boolean for which
they probably also defines aspecific version...

This may not be a bzrlib problem though...

As mentionned on IRC, I like the direction this is taking
regarding make_test_ui_factory being a good step towards a
fixture independent from the test itself.

Regarding 'bzr.lock.break.confirm', I'm fine with using an
arbitrary name space but sticking to the python module name space
may be better suited in the long term (easier to discover and to maintain).

Now, reagrding the relationships with config variables, we
can (hypothetically):

- define an alias between bzr and bzrlib or
  bzr.lock.break.confirm and bzrlib.lock.confirm_break or,
  even simpler, between bzrlib.lock.confirm_break and confirm_break_lock

- define (handwaving) a config var as bzrlib.lock.confirm_break
  and import it in bzrlib.lockdir

Or any combination.

Anyway, I now better understand what you mean for the application
code and indeed this looks nice.

Making the confirm_action implementation access the relevant
config from the ID may still be a big tricky though (wt, branch
?).

This may be addressed by setting some config attribute to the ui
though... unless we need the context manager for that...