[PATCH 2 of 2 V2] tests/run-tests: use hghave.py

[Matt Mackall]

On Mon, 2012-06-18 at 09:50 +0200, Adrian Buehlmann wrote:
> On 2012-06-18 03:57, Mads Kiilerich wrote:
> > Adrian Buehlmann wrote, On 06/14/2012 03:48 PM:
> >> # HG changeset patch
> >> # User Adrian Buehlmann <adrian at cadifra.com>
> >> # Date 1339677848 -7200
> >> # Node ID e08ba77235ac9abff2ab821b257e42abffa59553
> >> # Parent  b92c1e526b98c23de4f2fc4e16303e8171b8b861
> >> tests/run-tests: use hghave.py
> >>
> >> Eliminates the start of a subprocess for #if requirements checking.
> > 
> > I pushed patch 1 to crew, thanks.
> > 
> > But
> > 
> >> diff --git a/tests/hghave.py b/tests/hghave.py
> >> --- a/tests/hghave.py
> >> +++ b/tests/hghave.py
> >> @@ -272,3 +272,29 @@
> >>       "windows": (has_windows, "Windows"),
> >>       "msys": (has_msys, "Windows with MSYS"),
> >>   }
> >> +
> >> +class UnknownFeature(Exception):
> >> +     def __init__(self, f):
> >> +         self.f = f
> >> +     def __str__(self):
> >> +         return self.f
> > 
> > I am not convinced about this use of custom exception types for special 
> > (but exceptional) return values. It feels like a style we rarely use in 
> > Mercurial. But it is only the test runner and I don't see a better way 
> > to do it. I'm in doubt and would like to have a second opinion.
> 
> Heh. You don't see a better way but still don't want to take that? (But
> I'm grateful for the precision in how you stated your thinking. That's
> helpful). So I guess the second opinion is Matt?

Mads is right: the Mercurial codebase isn't big on creating
single-point-of-use classes. And more specifically, we're not fond of
creating new error classes as the vast majority of errors can't be
meaningfully "handled". We can only report errors nicely and do generic
cleanup, or ignore them. Which is why we use util.Abort rather than
creating error classes for most errors (and yes, I frankly think there's
too many classes in error.py).

This code, on the other hand, is actually using the exception as an
out-of-band communication scheme: it knows its single caller is going to
catch and handle it. Compare:

 try:
   good = features(l)
 except UnknownFeature, x:
   bad = [x]

and:

 good, bad = features(l)

If you're going to have this level of "collusion" between caller and
callee, exceptions just complicate things. On the other hand, exceptions
are a handy short-cut for deep or recursive call chains.

On the other hand, run-tests is certainly not an exemplar of good coding
style, and doesn't really particularly care about this sort of
robustness. I frankly think it's fine if unknown features raise a nasty
exception. Or if we catch KeyError. Or clone util.Abort. Or whatever.

> This is the way the Python folks explain in their docs how to define
> custom exceptions.

Yep, it definitely is the classic Python style, no arguing with that.

-- 
Mathematics is the supreme nostalgia of our time.