Code review comment for lp:~lifeless/bzr/fix-terminal-spew

It is a little bit odd to have an __exit__ that ignores the exception info passed to it, in the same way that "except: pass" tends to look a bit odd. As you say on IRC, "until we'd behave differently in an __exit__ call" then there's nothing much to do here. Certainly for the 'bzr' front-end we'd expect exceptions to be caught at this point... although if someone did use this in a 'with' statement in 2.5 it would stop e.g. KeyboardInterrupt propagating. So if the exc_* args are not None, I think BzrLibraryState.__exit__ should "return False" to tell 'with' statements to propagate the exception. And pass these arguments to the UI factory's __exit__, obviously... hmm, maybe should return it's true/false value?

I don't think that needs to be fixed before this can land — it's not a regression, and an overall improvement. But there is a small trap lurking here for advanced API users, so if it's not fixed then an XXX would be warranted.

The only other issue is documentation: bzrlib.initialize's docstring needs to describe its return value, and BzrLibraryState's docstring probably ought to mention that it implements the context manager introduced in Python 2.5. Either that, or you should add docstrings to __enter__ or __exit__, but I know which one is easier ;) It's a bit pedantic, but this is now a front door to the bzrlib API so it should be documented thoroughly and clearly.

Finally, I don't think you've addressed Martin[gz]'s concern about moving the UI cleanup earlier: in particular, if the call to sys.exitfunc() (i.e. to run the atexit hooks) causes something to use the UI, what happens? I think some of the -D flags that output end-of-run summaries (e.g. of HPSS call counts, or transport activity), might cause trouble here. Or maybe not, but I'd some reassurance about it :)