Merge lp:~jelmer/bzr/merge-dir-prober into lp:bzr

(I'm not looking for an in-depth review but would be interested to hear whether this is the right overall approach).

This seems fine in general, but I believe you're changing the signature of from_lines somewhat. Previously, it accepted an iterable, which could be a collection (i.e. list) or an iterator. Now, it seems a collection is mandatory because the iteration is repeated for every prober, but an iterator would be exhausted by the first prober.

Mandating lists may increase memory use, though of course, a given line could be twelve gigabytes if we were really unlucky. At the very least, you should change the docstring if you change the signature.

Because each prober is handled in sequence, you'll also be consuming the whole list of lines for every probe type, even if there's a header at the top of the file, indicating its kind.

Perhaps it would be better to us a coroutine approach instead. For example:

def get_merge_directive_type(lines):
    for line in lines:
        for prober in probers:
            prober.consume(line)
            if prober.merge_directive_type is not None:
                return prober.merge_directive_type

> This seems fine in general, but I believe you're changing the signature of
> from_lines somewhat. Previously, it accepted an iterable, which could be a
> collection (i.e. list) or an iterator. Now, it seems a collection is
> mandatory because the iteration is repeated for every prober, but an iterator
> would be exhausted by the first prober.

Just to note, until I rewrote it in r4792.7.3 from_lines did in fact require a list despite the docstring claiming it took an iterable, so changing the contract to actually require a list is unlikely to cause any kind of regression.

On Fri, 2010-08-06 at 19:58 +0000, Martin [gz] wrote:
> > This seems fine in general, but I believe you're changing the signature of
> > from_lines somewhat. Previously, it accepted an iterable, which could be a
> > collection (i.e. list) or an iterator. Now, it seems a collection is
> > mandatory because the iteration is repeated for every prober, but an iterator
> > would be exhausted by the first prober.
>
> Just to note, until I rewrote it in r4792.7.3 from_lines did in fact
> require a list despite the docstring claiming it took an iterable, so
> changing the contract to actually require a list is unlikely to cause
> any kind of regression.
Ahh, thanks for letting me know. I thought that was the case so was
surprised to find it was using an iterator now.

Cheers,

Jelmer

Hi Aaron,

On Thu, 2010-08-05 at 02:42 +0000, Aaron Bentley wrote:
> This seems fine in general, but I believe you're changing the signature
> of from_lines somewhat. Previously, it accepted an iterable, which
> could be a collection (i.e. list) or an iterator. Now, it seems a
> collection is mandatory because the iteration is repeated for every
> prober, but an iterator would be exhausted by the first prober.
>
> Mandating lists may increase memory use, though of course, a given line
> could be twelve gigabytes if we were really unlucky. At the very
> least, you should change the docstring if you change the signature.
>
> Because each prober is handled in sequence, you'll also be consuming
> the whole list of lines for every probe type, even if there's a header
> at the top of the file, indicating its kind.
>
> Perhaps it would be better to us a coroutine approach instead. For example:
That's a good point, for some reason I thought that it scaled with the
size of the file anyway. Using coroutines does indeed seem like a good
approach here.

Thanks for the review.

Cheers,

Jelmer