[MERGE] Make log revision filtering pluggable.

[Robert Collins]

On Tue, 2008-08-26 at 16:38 -0400, John Arbash Meinel wrote:
> John Arbash Meinel has voted comment.
> Status is now: Semi-approved
> Comment:
> I'm a bit concerned about your api. You seem to have an iterator of 
> iterators, which is a bit confusing in its nesting.

This is for performance, to allow the current batching of revisions to
be presented to the filters, rather than them all reinventing it, etc.

> Like this:
> +        def _convert():
> +            for view in view_revisions:
> +                yield (view, None, None)
> +        log_rev_iterator = iter([_convert()])
> 
> If I understand it correctly you need to do a double for loop on that. 
> Such as
> 
> for iterator in log_rev_iterator:
>    for revision_id, revno, depth in iterator:
>      do stuff
> 
> At a minimum, it would be nice to have it more clearly documented.

Ok, can you suggest where ? The docstring is accurate, and there are
many examples of working with it.

> It also seems like the function would be:
> make_log_rev_iterators (note the plural iterators).
> 
> And then "for rev_iter in revision_iterators:"
> etc.
> 
> So... I really like it being a chain of filters. But I wonder if we 
> would be a lot better served by having a chain of Classes rather than 
> plain functions. At least at the moment, I have difficulty following 
> what is really going on. I can sort of tease it out, but it doesn't 
> really pass my "simplicity" filter.

How would a chain of classes work any different? This is quite literally
exactly what we had for log before (though it was a bit inconsistent
before - some levels were single rev, some were multiple).

> Oh, and if "log_adapters" is going to be non-stable, I think it should 
> be "_log_adapters" for now.

_log_adapters would be private. This is public, but not stable.

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080827/a5ba1557/attachment.pgp