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

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

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

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.

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

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

At least internally, but the caller itself may use an aliased form.

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

Yup, hence 'setting some config attribute to the ui' not 'add a
parameter to confirm_action'. There is certainly some risks down that
path regarding getting the setting correct (if some method set the
config to repo but is also called when the config is set to wt should it
override or not ?) but it's too vague in my mind to really know yet.

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

Oh, you said: "Before landing I want to update the callers but I'd
appreciate feedback now on the api." so I was waiting for the real
submission to review :-)