Code review comment for lp:~gz/bzr/smarter_smart_error_reporting_722416_B

Martin [gz] wrote:
[...]
> > It's probably a good idea for the error to say which remote server, though.
> > Various scripts, plugins, stacking configurations, etc, may have multiple
> > connections active at one time, and the user probably wants to know which one
> > had the problem :)
>
> That's a good point. Though... ☃ asks doesn't that mean we do want
> context then? It also goes for the other general errors that are being
> re-raised, so perhaps a different approach might be helpful in the
> future.

I suppose you're right! Although there's sort of different levels of
context...

I think part of the reason I had the distinct message._translate_error
vs. remote._translate_error functions was to keep knowledge of random
bzr model stuff out of the basic protocol. i.e. there's no reason why
the message layer, which is about taking simple structures off the wire
and turning them into events like “here's a request body” should know
anything about what NoSuchRevision means.

I don't think I thought through that particular refactoring carefully at
the time, and so conflated “doesn't need context stuff from remote.py”
with “message-level logic.” But it was a step towards that, and now
with your patches I think it's actually pretty right: the message layer
knows that an error is either UnknownMethodError, or else it is a
ErrorFromSmartServer for a higher layer to interpret, and that's it.

It's not really a goal, but if you like you can imagine a design
principle here as being “the smart protocol stuff could be used to write
useful servers and clients that have nothing to do with bzr.” So
bzrlib.smart.protocol, bzrlib.smart.message, bzrlib.smart.client and
most of bzrlib.smart.request for instance are pretty generic and on
nothing-to-do-with-the-rest-of-bzr side of the fence, while
bzrlib.remote and bzrlib.smart.bzrdir, etc, clearly belong to other
side.

That's a long winded way of saying: yes, I think you're right that the
medium or its URL or something probably belongs in the context, and that
the MemoryError translation (and possibly others) should use that.

And you're making me think that the no-context distinction isn't
important at all (now that I've figured out that it was the “belongs in
bzrlib.smart.message or not” distinction that I really cared about, even
though I had it a bit wrong).

-Andrew.