Overriding commands in plugins

[Michael Gliwinski]

On Tuesday 09 Mar 2010 23:06:15 Martin Pool wrote:
> On 9 March 2010 19:56, Michael Gliwinski
> 
> <Michael.Gliwinski at henderson-group.com> wrote:
> > On Tuesday 09 Mar 2010 06:42:45 Martin Pool wrote:
> >> On 9 March 2010 04:06, Gary van der Merwe <garyvdm at gmail.com> wrote:
> >> > On 08/03/2010 17:18, Michael Gliwinski wrote:
> >> >> What is the best/proper/right/working way to override some
> >> >> functionality of a command in a plugin?
> >> >
> >> > I looked into this in detail when trying to solve the incompatibility
> >> > between qbzr and bzr-pipeline which were both trying to modify the
> >> > merge command. I came to the conclusion that it's not possible to do
> >> > cleanly with how bzr currently is. I started making changes to bzr to
> >> > make this possible[1], but ran out of motivation, and qbzr ended up
> >> > losing some functionality (merge --qpreview)
> >> >
> >> > [1]:
> >> > https://code.launchpad.net/~garyvdm/bzr/register_lazy_decorated/+merge
> >> >/84 30
> >>
> >> The easiest way is to override the plugin altogether and super-call it.
> >
> > I'm not sure what you mean by that.
> 
> I meant to subclass the base implementation, then call super(...).run()

Ah yes, that is indeed the easiest but my point was that it bypasses other 
plugins that might also modify the same command and AFAICT which plugin "wins" 
would be quite random.

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

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?

Not that in this case I specifically need to ask interactively, I could go with 
adding another option to command and configuration file (e.g. 
i_know_what_switch_is_doing = True ;) ), just asking out of curiosity.

> > Also, there is the other use case (apart from extending a command) of
> > replacing functionality depending on certain conditions, that's what e.g.
> > loom does with switch command.
> >
...
> >
> > My use case now is to have switch warn interactively if there are
> > uncommited changes/unknown files.  I know it is perfectly valid for
> > switch to just propagate these as it does now, but I have a lot of users
> > here (most quite new to using VCS at all) who get bitten by this quite
> > often (e.g. forgetting to run status often and after a couple of switches
> > ending up with conflicts in conflict files, etc.).
> 
> 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.)

> > I noticed the command hooks yesterday, from what I understand they are
> > actually run when the command object is created, yes?  Would that be
> > valid to wrap the run method in a hook?
> 
> As Robert says, you could do it, but it seems a bit of a blunt instrument.

I see your and Robert's point and I agree.  Hmm, need to think about it some 
more :)

Thanks,
Michael


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