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

On 10 September 2010 02:19, Vincent Ladeuil <email address hidden> wrote:
> 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).

Thanks for the speedy review.

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.

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

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

I'll cc them and they may have a thought.

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

So bzrlib.lockdir.break, or something similar?

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

Interesting point. istm that you might want per-context configuration
to affect the ui in more than just confirmations...

So is the overall vote here 'tweak, plus check with gui maintainers,
plus update the other callers'?

--
Martin