Exception mangling in hooks?

[Robert Collins]

On Fri, 2009-02-27 at 21:50 +1100, Andrew Bennetts wrote:
> Robert Collins wrote:
> > On Thu, 2009-02-26 at 08:31 -0600, John Arbash Meinel wrote:
> [...]
> > > History has shown that plugins do tend to be sloppy about error
> > > handling, and we have gotten a fair number of bug reports because of
> > > things that plugins do. (I don't have a count, but as an example just
> > > bzr-loom's handling of 'status' caused 3-4 repeat bugs to be raised in
> > > bzr core, even though the last line clearly showed it was occurring in
> > > bzr-loom.)
> > 
> > This is true, but loom was failing in command argument handling, not in
> > a hook. If the general principle were a good idea, we'd be wrapping
> > every exception that a plugin might generate, no?
> 
> That's hardly a convincing argument.  There are plenty of good ideas we
> haven't implemented yet...

I was trolling, I think its a terrible idea.

> FWIW, I agree with John — and I suspect custom hooks are going to be more
> likely to be buggy than general plugins, because they are/will be a more
> common sort of plugin to write, to meet the specific needs of a particular
> project.  They're more likely to be written on an ad hoc basis and not
> rigorously tested.
> 
> There's a limit to how much we can protect bzrlib against plugins.  New
> branch formats like bzr-loom's integrate at a very deep and pervasive level,
> so the best we can realistically do for that case is something like tweak
> bzrlib.trace.report_bug to examine the traceback we're about to show the
> user and modify our “Please report a bzr bug” message to be a bit more
> ambivalent if “bzrlib/plugins” appears in the traceback.

If we want to do that, we can do it with hooks too.

> But when it comes to hooks, they are clear, well-defined points where we're
> handing off to 3rd-party code.  It's pretty easy to intercept their errors
> so that we report them more clearly for users (and insulate bzrlib from
> accidentally misinterpreting a hook's exception as one of its own), and I
> think the downside is minimal.  (And if a particular hook wants to
> explicitly allow some exceptions, as pre_change_branch_tip does with
> TipChangeRejected, that's easily accomodated.)

It makes hooks into a special case where there isn't a need or (IMO) a
benefit to one. You can't for instance raise any exception (such as
AssertionError) in one of these hooks - it will get translated into a
different exception. It makes hooks into a second class citizen rather
than part of the normal functioning of bzrlib.

So working with them requires working differently than elsewhere in
bzrlib, and this is exacerbated when we don't do it consistently.

> I'm not sure why you found the HookFailed exception so much harder to work
> with when diagnosing test failures: the full traceback should still be
> displayed in a test failure, because Python shows "traceback +
> str(exception)", and str(exception) here will include the traceback captured
> by HookFailed.  The captured traceback is still accessible in the HookFailed
> object if you want to use pdb on it.  We could add a .post_mortem method to
> HookFailed invoke pdb on it if that's what you need, but I honestly don't
> follow why you found it harder to work with.  I originally added it because
> I found it clearer and easier!

What did adding this wrapped clearer for you (and how?). I don't want to
remove something thats useful; I just don't find it useful, and do find
it in the way.

> Even if there is some degree of developer inconvenience that can't be
> improved, that should be weighed against the better user experience.

I am not convinced that the user experience is better.

There are two groups of users here - those that install other peoples
plugins, and those that write plugins.

The first group have to file a bug regardless, and the second group are
best served by the raw exception - IDE's know to jump to the actual
point of the failure, not the re-raised line, if you have the original
exception. The original exception could have args that are valuable, and
it can be useful to the caller.

-Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090228/aafe099f/attachment-0001.pgp