Overriding commands in plugins

[Martin Pool]

On 18 March 2010 02:37, Michael Gliwinski
<Michael.Gliwinski at henderson-group.com> wrote:
>> >> 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.

Just so.  It seems to me that if the only extension available is "wrap
the whole command" you are unavoidably going to find it hard for
multiple plugins to do interesting extensions.

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

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

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

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.

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

-- 
Martin <http://launchpad.net/~mbp/>