[MERGE] setlocale (#128496)

[Martin Pool]

On 6/17/08, Martin von Gagern <Martin.vGagern at gmx.net> wrote:
> Aaron Bentley wrote:
> > > Notice that a KeyboardInterrupt is a BaseException but not an instance
> > > of Exception,
> >
> > That change was made in python2.5.  But we support 2.4.
> > KeyboardInterrupt is a subclass of StandardError on python 2.4

My original mail was a bit truncated: it's true that we shouldn't be
catching all these errors, but a more important point is that we
generally don't want to catch errors "just in case".  The exceptions
are well defined layers where that's clearly part of their job eg to
print the error to the user or to cancel an operation.  We've
generally found if you do this it tends to hide bugs from testing and
also from the developer.

So what I really meant to ask was, why was that code like it is.

Looking at it again, clearly it's not just about printing strings but
also about any object that might have a __str__ or __repr__ method,
and because those can run arbitrary code they can raise arbitrary
errors.  (In fact a similar topic came up recently, where I said that
perhaps the methods themselves should have this catch block.)

If the str or repr is failing we probably don't want to silently
absorb that problem.  At the very least we should give a warning.

The other kind of conceptual issue is that this is unclear about
whether it's trying to detect problems in unicode conversion or in
running __str__.  I can see in some cases you would want both but
other places will know they have a string already; or know that they
have an object that must be printable.  For example it's not clearly
good that passing an object to assertEqualDiff should magically
compare it's string value.

I guess generally for internal interfaces we try to avoid a 'do what i
mean' or defensive style.  Things should be called with a value of one
particular type and just fail if they don't get it.  Otherwise it can
hide errors, make the api harder to evolve, or hurt performance.  So
generally rather than the try/except blocks in this code, I'd prefer
to say that these things should for example just always print an
escaped form, or always not.

> >
>
>  Hmmm... OK, so what to do now? We have several options.
>
>  1. Leave it as it is in my patch
>  2. Reduce the kinds of errors caught to TypeError and ValueError
>  3. Add special handling for KeyboardInterrupt and reraise that
>  4. Drop _safe_string and accept that broken reports might break the test
> suite from time to time
>
>  There are several arguments why 1. is not really as bad as it might seem at
> first glance:
>
>  a) Only affects test suite, not production uses
>  b) Function only called if a test fails and its report won't convert, so it
> should never get called in the first place
>  c) Only a problem on Python < 2.5
>  d) String conversion shouldn't be time consuming, so interrupting just
> while the conversion is running should be extremely unlikely
>  e) Will not loop endlessly, a maximum of two such exceptions get caught per
> invocation.
>
>  Come to think of it, it might even be considered a feature that aborting a
> lengthy (and thus probably broken) conversion of an object to string can be
> interrupted without aborting the whole test suite.
>
>  Although I'm willing to implement any of my suggestions, I personally would
> leave it as it is in my patch, and not invest too much effort and lines of
> code into a corner case that has little to no practical impact.
>
>  If you are interested, I can later try to reproduce the errors that
> originally led me to introduce this thing, so you can see how individual
> test cases can break the whole suite under certain circumstances.

I'd like to see that, if you can remember or reproduce it.  In general
they should not.  It may be that if an error occurs printing some
information about a test, like the traceback or test name, then the
suite terminates.

Thanks

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