[PATCH 5 of 7] revset: introduce a filterrevs function

[Pierre-Yves David]

On 23 juin 2012, at 03:35, Augie Fackler wrote:

> 
> On Jun 22, 2012, at 4:45 PM, Pierre-Yves David wrote:
> 
>>> +    except (AttributeError, TypeError, KeyError):
>>> +        pass
>>> +    return filter(subset.__contains__, revs)
>> 
>> You seems to like the try:..except: pattern for common code path. It's look like a bad idea. I do not know how awful the code is with standard if but having such a wide silent except "AttributeError", "TypeError", and "KeyError" seems a perfect way to loose hours in debugging later.
>> 
>> Moreover the code path is very implicit and then hard to follow
> 
> Very often this performs significantly better than the alternatives, especially in cases where the exception-based codepath is actually exceptional.

My main concern here is that 

(1) This offuscate the code a lot: because (and only because) I read the preview series I know that this function take: list, set, repo. The new range object. And I'm not sure of which raise what, when. And each possibility is not "rare".

Anybody that get into this code from the outside will be very confuse. Especially given the lack of doc.

If the speedup is really worse-while, this really need a proper documentation of which operation may raises what Exception.

(2) AttributeError, TypeError, KeyError are very wide error that may make later debugging an headache


My 2 cents as someone who will probably descent into revset code one day or another.

-- 
Pierre-Yves David