[RFC] Allow error definitions outside bzrlib.errors?

[Martin Pool]

2009/8/11 Andrew Bennetts <andrew.bennetts at canonical.com>:
> For a long time our policy has been that error definitions belong in
> bzrlib.errors.  This has been very convenient, but we've also found that it's a
> wasteful to have every invocation of Bazaar execute the definitions of over 300
> error classes when most invocations will use virtually none of them.  The cost
> isn't huge (roughly 20ms according to a quick-and-dirty benchmark on my laptop),
> but it's large enough that we've become reluctant to keep adding to errors.py.
>
> So, I propose we relax the policy a little.  I suggest we allow defining some
> errors inside the modules that raise them, for errors that:
>
>  a) raised only by one module, and
>  b) typically the only places that explicitly catch that error are the
>     immediate callers of that module.  (“explicitly” meaning caught via “except
>     ExactErrorName” or similar, rather than catch-alls like the one found in
>     run_bzr_catch_user_errors).

That sounds ok to me.

It's a bit similar to the Python standard library approach: there are
some global/standard errors, and then module-specific errors a bit
like what you describe.

If no one objects and we go ahead with this, could you merge a tweaked
version of this mail into the developer docs, updating whatever may
already be there?

> Here's the example that prompted this: the bzrlib.inventory_delta is currently
> returning generic BzrErrors in lieu of returning more specific exceptions
> because we didn't want to pay the price of yet another definition in
> bzrlib.errors.  But a more specific exception (or exceptions!) would certainly
> be useful.  The cases in this module (ignoring really basic
> AssertionErrors/TypeErrors) are:
>
>  - failure to serialize a delta (presumably because the delta is not sane, or
>    does not meet expectations about having/not having a versioned root, etc.)
>  - failure to deserialize a delta (due to inconsistent serialized data, or
>    processing an incomplete or corrupted serialization, or processing something
>    that simply isn't an inventory delta)
>  - deserializing an incompatible delta (such as encountering a tree reference
>    when the caller has said it does not allow them).
>
> Of these, the first is probably a sign of a bug in bzrlib, the second is
> probably an error in the input and should be reported to the user, and the third
> is particular kind of error in the input that should probably be mapped to
> bzrlib.errors.IncompatibleRevision by the caller (because the caller presumably
> has a repository object to construct IncompatibleRevision with).  Of these
> cases, the one I most want a distinct exception for is the third, so that
> StreamSink (the only non-test caller of the deserializer in my inventory-delta
> branch) can catch it and make an IncompatibleRevision error from it.  But having
> even a single InventoryDeltaError would improve the precision of the
> inventory_delta unit tests significantly.

Right - this too probably deserves to be documented (more).  We don't
need a new class for every place that can raise an error, only if the
caller might want to catch it and distinguish it, which is plausible
for the third case.

Though even then, perhaps it should be some more generic "incompatible formats".

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