[MERGE] Remove duplication of error translation in bzrlib/remote.py.

Andrew Bennetts andrew at canonical.com
Wed Jul 23 00:09:55 BST 2008


John Arbash Meinel wrote:
[...]
> This would seem like a great place to directly test the translation
> functionality.

Good idea.  I'll add unit tests for the _translate_error function, one for each
error code.

> I'm a bit concerned that a typo somewhere, or a missing context is going
> to cause a KeyError rather than raising the correct exception, just
> without the expected context.
>
> I suppose that might make it more obvious that it is happening, and get
> us to fix it, though. If I was debugging, I'd want the KeyError. If I
> was a *user* I'd want the regular exception.

I already have the 'find' helper nested function to reduce the risk of a useless
error.  So if some context is missing, in most cases that will just cause things
like “(unknown branch)” in the error rather than the real branch.  It would be
good to be more robust though.  I'm probably being a bit optimistic that
substuting a str for a real object is going to always produce an acceptable
exception.

If the context is missing a required key, then displaying the
ErrorFromSmartServer is likely to at least be a bit helpful, certainly more than
a KeyError from _translate_error.  So I think the right thing to do is never use
the context dict directly, and instead always go through this 'find' helper:

    def find(name):
        try:
            return context[name]
        except KeyError, keyErr:
            mutter('Missing key %r in context %r', keyErr.args[0], context)
            raise err

This way developers will see the details of the bug in the ~/.bzr.log, and
ordinary users will get the original ErrorFromSmartServer.  That means a
traceback, but at least it has an ok chance of being useful.  Which is a pretty
good result for a bug :)

So, I've changed the branch to do that.

Thanks for the review.

-Andrew.




More information about the bazaar mailing list