Overriding commands in plugins

[Michael Gliwinski]

On Thursday 18 Mar 2010 05:40:28 Martin Pool wrote:
> On 18 March 2010 02:37, Michael Gliwinski
...
> >> >> The best way is to (ask for help to) change the command so that it
> >> >> can more easily be extended in different directions; this is the only
> >> >> feasible way to accommodate different plugins all wanting to enhance
> >> >> eg the merge command.
> >> >
> >> > Yes, I agree, e.g. callbacks at certain points of the operation would
> >> > make things simple.  I believe bzrlib uses hooks for this kind of
> >> > extensibility at lower levels (than commands), are you thinking of
> >> > similar/same infrastructure for commands as well?
> >>
> >> Yes, I think that would be reasonable.  We could add hooks run at the
> >> start and end of commands, which would allow some types of extension.
> >> But to do more interesting things you would want a hook that runs at
> >> interesting points during the command.  And really many of these
> >> probably don't want to be in the CLI in particular so much as at a
> >> somewhat lower level.  For instance you would perhaps want your hook
> >> to also run if the "switch" operation was run through some other UI.
> >
> > Regarding hooks that would run at interesting points during the command,
> > would those "interesting points" have to be defined generically for all
> > commands (because hooks are on Command class)?  Or is there a way to make
> > each command class have its own hooks?  Would that even be desirable?
> 
> Perhaps some could be generic, like "after opening the source" but I
> think the most interesting ones need to be specific to commands, or at
> least to groups of similar commands.

That sounds good.  However also slightly complicated when thinking about 
implementation.  Is there a concept of "groups of commands" in bzr already or 
would that require a refactor somewhere?

> > Also, I agree in this case (and similar ones) it would make more sense
> > for it to be done at lower level so other UIs and as Robert mentioned
> > other conceptually similar things are handled as well.  The problem is
> > that the lower level shouldn't really be doing UI kinds of things, e.g.
> > interactively asking for confirmation.  Is there a way to express such
> > things in UI-agnostic way anyways?
> 
> You're right the lower levels shouldn't be directly dealing with the
> UI, but I think it's reasonable for them to say to the UIFactory "I'm
> doing something the user might want to know about or confirm", and
> then for it to if appropriate ask.  That gives us a central place to
> suppress those warnings, to test them, and potentially to hook them.
> I think the cross-format fetch warning is fairly tasteful.

OK, just finished reading bzrlib.ui and qbzr's uifactory.  All makes sense now.  
So this should be possible with addition of e.g. 'default' param to 
ui_factory.get_boolean, I think.  This would allow e.g.:

if ui_factory.get_boolean('Encountered foo, abort?', default=True):
    raise # something to abort

or maybe a separate method to allow for more verbose description of the 
condition?  I can imagine (for my case) wanting to pass some object 
representing state of the tree and having ui_factory figure out how to present 
it.

...
> >> I think this probably ought to be in a WorkingTree.pre_switch hook?
> >>
> >> If other developers feel this would be useful I can help you add it.
> >
> > OK, so I had a look at how switch is implemented and I'm not quite sure
> > how pre_switch hook could be implemented.  IIUC switch isn't as really an
> > operation on the WorkingTree, it actually changes branch location in the
> > control directory, pulls the branch (if any) and then updates the
> > WorkingTree.
> >
> > Would anyone actually be interested in that?  Cause for my use case I can
> > just hook into the CLI command (my users don't use GUIs nor more complex
> > things like looms, etc.)
> 
> So in this particular case we have an intermediate layer bzrlib.switch
> and it would be reasonable to run the hook from there.  As you say
> there are actually two phases, first switching to the other branch and
> then updating the wt.  It would be reasonable to have a hook on the wt
> that can interrupt before the update, but really you want to catch
> this before anything is done, so probably a hook run near the top of
> _set_branch_location would be good.
> 
> It's not as obvious as wrapping the command but it would be a step in
> a very good direction and we will help you if you have a go at it.

OK, so just to make sure I got this right.  So you mean adding a HookPoint 
'pre_switch' to MutableTreeHooks and invoking that hook from bzrlib.switch 
somewhere, yes?  Maybe directly in bzrlib.switch.switch then, e.g. 
before/after _check_pending_merges?

Right, anyways, I think I'm going to start by logging feature requests for 
those three things in LP and we'll go from there.  I'll find some time to have 
a look at at least the pre_switch hook and ui_factory change.


-- 
Michael Gliwinski
Henderson Group Information Services
9-11 Hightown Avenue, Newtownabby, BT36 4RT
Phone: 028 9034 3319

**********************************************************************************************
The information in this email is confidential and may be legally privileged.  It is intended solely for the addressee and access to the email by anyone else is unauthorised.
If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.
When addressed to our clients, any opinions or advice contained in this e-mail are subject to the terms and conditions expressed  in the governing client engagement leter or contract.
If you have received this email in error please notify support at henderson-group.com

John Henderson (Holdings) Ltd
Registered office: 9 Hightown Avenue, Mallusk, County Antrim, Northern Ireland, BT36 4RT.
Registered in Northern Ireland
Registration Number NI010588
Vat No.: 814 6399 12
*********************************************************************************